Refactor textToQualifiedNameList()

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Refactor textToQualifiedNameList()

Yugo Nagata
Hi,

When working on other patch[1], I found there are almost same
functions, texttoQualifiedNameList() and stringToQualifiedNameList().
The only difference is the argument type, text or char*. I don't know
why these functions are defined seperately, but I think the former
function can be rewritten using the latter code as the attached patch.
Is this reasonable fix?

Regards,
--
Yugo Nagata <[hidden email]>

refactor_textToQualifiedNameList.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Yugo Nagata
On Fri, 24 Aug 2018 20:44:12 +0900
Yugo Nagata <[hidden email]> wrote:

> Hi,
>
> When working on other patch[1], I found there are almost same
> functions, texttoQualifiedNameList() and stringToQualifiedNameList().
> The only difference is the argument type, text or char*. I don't know
> why these functions are defined seperately, but I think the former
> function can be rewritten using the latter code as the attached patch.
> Is this reasonable fix?

I forgot to add a link to the referred thread.

[1] https://www.postgresql.org/message-id/20180817075100.bc99378255943d3c3951ad63%40sraoss.co.jp

>
> Regards,
> --
> Yugo Nagata <[hidden email]>


--
Yugo Nagata <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Kyotaro HORIGUCHI-2
In reply to this post by Yugo Nagata
Hello.

At Fri, 24 Aug 2018 20:44:12 +0900, Yugo Nagata <[hidden email]> wrote in <[hidden email]>
> When working on other patch[1], I found there are almost same
> functions, texttoQualifiedNameList() and stringToQualifiedNameList().
> The only difference is the argument type, text or char*. I don't know
> why these functions are defined seperately, but I think the former
> function can be rewritten using the latter code as the attached patch.
> Is this reasonable fix?

The functions were introduced within a month for different
objectives in March and April, 2002. I supppose that they are
intentionally added as separate functions for simplicitly since
the second one is apparent CnP'ed from the first one.

commit 5f4745adf4fb2a1f933b25d7a2bc72b39fa9edfd
commit 52200befd04b9fa71da83231c808764867079226

Returning to the patch, the downside of it is that textToQNL
makes an extra and unused copy of the parameter string. (It's a
kind of bug that it is forgetting to free rawname.)

Maybe we can separate them into three functions (or one function
and two macros) to get rid of the duplication but I'm not sure
it's worth doing..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Yugo Nagata
On Tue, 28 Aug 2018 11:49:26 +0900 (Tokyo Standard Time)
Kyotaro HORIGUCHI <[hidden email]> wrote:

> At Fri, 24 Aug 2018 20:44:12 +0900, Yugo Nagata <[hidden email]> wrote in <[hidden email]>
> > When working on other patch[1], I found there are almost same
> > functions, texttoQualifiedNameList() and stringToQualifiedNameList().
> > The only difference is the argument type, text or char*. I don't know
> > why these functions are defined seperately, but I think the former
> > function can be rewritten using the latter code as the attached patch.
> > Is this reasonable fix?
>
> The functions were introduced within a month for different
> objectives in March and April, 2002. I supppose that they are
> intentionally added as separate functions for simplicitly since
> the second one is apparent CnP'ed from the first one.
>
> commit 5f4745adf4fb2a1f933b25d7a2bc72b39fa9edfd
> commit 52200befd04b9fa71da83231c808764867079226
Thank you for your comments. I also looked at the commit history.
stringToQNL is added after textToQNL as a static function originally,
but this is now public and reffered from other modules, too.  So, I
propose to mote this from regproc.c to verlena.c and rewrite textToQNL
to call stringToQNL.  This is just for reducing redundancy of the code.
Attached is the updated patch.
 
> Returning to the patch, the downside of it is that textToQNL
> makes an extra and unused copy of the parameter string. (It's a
> kind of bug that it is forgetting to free rawname.)

I also fixed to free rawname in the texttoQNL.

Regards,

--
Yugo Nagata <[hidden email]>

refactor_textToQualifiedNameList_v2.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Pavel Stehule
The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, failed
Spec compliant:           tested, failed
Documentation:            tested, failed

Hi

I tested this patch. This patch removes some duplicate rows, what is good -  on second hand, after this patch, the textToQualifiedNameList does one more copy of input string more. I looked where this function is used, and I don't think so it is a issue.

This patch is trivial - all tests passed and I don't see any problem. I'll mark as ready for commit.

The new status of this patch is: Ready for Committer
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Michael Paquier-2
On Mon, Oct 08, 2018 at 03:22:55PM +0000, Pavel Stehule wrote:
> I tested this patch. This patch removes some duplicate rows, what is
> good -  on second hand, after this patch, the textToQualifiedNameList
> does one more copy of input string more. I looked where this function
> is used, and I don't think so it is a issue.
>
> This patch is trivial - all tests passed and I don't see any
> problem. I'll mark as ready for commit.
>
> The new status of this patch is: Ready for Committer

Are you sure that there is no performance impact?  Say implement a
simple SQL-callable function which does textToQualifiedNameList on the
same string N times, and then compare the time it takes to run HEAD and
the refactored implementation.  I may buy that an extra palloc call is
not something to worry about, but in my experience it can be a problem.

This patch also changes stringToQualifiedNameList from regproc.h to
varlena.h, which would break extensions calling it.  That's not nice
either.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Pavel Stehule
Hi

út 9. 10. 2018 v 5:28 odesílatel Michael Paquier <[hidden email]> napsal:
On Mon, Oct 08, 2018 at 03:22:55PM +0000, Pavel Stehule wrote:
> I tested this patch. This patch removes some duplicate rows, what is
> good -  on second hand, after this patch, the textToQualifiedNameList
> does one more copy of input string more. I looked where this function
> is used, and I don't think so it is a issue.
>
> This patch is trivial - all tests passed and I don't see any
> problem. I'll mark as ready for commit.
>
> The new status of this patch is: Ready for Committer

Are you sure that there is no performance impact?  Say implement a
simple SQL-callable function which does textToQualifiedNameList on the
same string N times, and then compare the time it takes to run HEAD and
the refactored implementation.  I may buy that an extra palloc call is
not something to worry about, but in my experience it can be a problem.

I try to find some worst case where this function is used and I expect so most critical usage is in "nextval" function.

I disabled fsync and debug assertions and created sequence by command

create sequence pretty_long_schema_name.pretty_long_sequence_name cache 1000;

And I tested

explain analyze select nextval('pretty_long_schema_name.pretty_long_sequence_name') from generate_series(1,10000000);

The difference on 10M calls is about 300ms - it is about 6%.

It is probably the cost of one palloc and one free more.

I am not able to decide if it is too much. If it is too much, then probably is better do nothing. Introducing another third function we will have more lines than before.
 

This patch also changes stringToQualifiedNameList from regproc.h to
varlena.h, which would break extensions calling it.  That's not nice
either.

It is not nice, but this is not serious problem for extensions's authors. I remember more similar issues when I worked on Orafce extension.

My opinion is neutral - this patch reduces some lines, but not too much, and has some overhead, but not too significant. Maybe more rich comment or notes can be best solution.

Regards

Pavel
--
Michael
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Michael Paquier-2
On Tue, Oct 09, 2018 at 10:47:48AM +0200, Pavel Stehule wrote:
> The difference on 10M calls is about 300ms - it is about 6%.

This number gives a good argument for rejecting this patch.  I am not
usually against code beautification, but that's a high price to pay for
just some refactoring.  On top of that, this potentially breaks
extension compilation.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Pavel Stehule


út 9. 10. 2018 v 13:20 odesílatel Michael Paquier <[hidden email]> napsal:
On Tue, Oct 09, 2018 at 10:47:48AM +0200, Pavel Stehule wrote:
> The difference on 10M calls is about 300ms - it is about 6%.

This number gives a good argument for rejecting this patch.  I am not
usually against code beautification, but that's a high price to pay for
just some refactoring.  On top of that, this potentially breaks
extension compilation.

ok

Regards

Pavel

--
Michael
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Alvaro Herrera-9
In reply to this post by Michael Paquier-2
On 2018-Oct-09, Michael Paquier wrote:

> On Tue, Oct 09, 2018 at 10:47:48AM +0200, Pavel Stehule wrote:
> > The difference on 10M calls is about 300ms - it is about 6%.
>
> This number gives a good argument for rejecting this patch.  I am not
> usually against code beautification, but that's a high price to pay for
> just some refactoring.  On top of that, this potentially breaks
> extension compilation.

One thing I do like about this patch is that it takes
stringToQualifiedNameList out of regproc.c, where it was put as static
by commit 52200befd04b ("Implement types regprocedure, regoper,
regoperator, regclass, regtype" April 2002); made extern by ba790a5608ea
("Here is a patch for Composite and Set returning function support."
June 2002) in what appears to have been a careless change.  The function
would end up in a place where it more reasonably belongs into,
varlena.c, next to its sibling textToQualifiedNameList.

The committer of such a change will get a lot of flak for changing the
#include requirements for code that calls that function, though.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Michael Paquier-2
On Tue, Oct 09, 2018 at 09:54:12AM -0300, Alvaro Herrera wrote:
> The committer of such a change will get a lot of flak for changing the
> #include requirements for code that calls that function, though.

So the patch has been switched to rejected...
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Alvaro Herrera-9
On 2018-Oct-10, Michael Paquier wrote:

> On Tue, Oct 09, 2018 at 09:54:12AM -0300, Alvaro Herrera wrote:
> > The committer of such a change will get a lot of flak for changing the
> > #include requirements for code that calls that function, though.
>
> So the patch has been switched to rejected...

I know -- I'm just disagreeing with that.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Refactor textToQualifiedNameList()

Michael Paquier-2
On Wed, Oct 10, 2018 at 09:45:22AM -0300, Alvaro Herrera wrote:
> On 2018-Oct-10, Michael Paquier wrote:
>> On Tue, Oct 09, 2018 at 09:54:12AM -0300, Alvaro Herrera wrote:
>>> The committer of such a change will get a lot of flak for changing the
>>> #include requirements for code that calls that function, though.
>>
>> So the patch has been switched to rejected...
>
> I know -- I'm just disagreeing with that.

The point of moving stringToQualifiedNameList out of regproc.c to
varlena.c is actually a good one, if we don't add the extra palloc
overhead.  Now I suspect that any change of header makes plugin
maintainers unhappy, and I can see this routine extensively used on
github, like pipelineDB or such.
--
Michael

signature.asc (849 bytes) Download Attachment