Thread: Refactor textToQualifiedNameList()
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 <nagata@sraoss.co.jp>
Attachment
On Fri, 24 Aug 2018 20:44:12 +0900 Yugo Nagata <nagata@sraoss.co.jp> 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 <nagata@sraoss.co.jp> -- Yugo Nagata <nagata@sraoss.co.jp>
Hello. At Fri, 24 Aug 2018 20:44:12 +0900, Yugo Nagata <nagata@sraoss.co.jp> wrote in <20180824204412.150979ae6b283ddb639f93f6@sraoss.co.jp> > 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
On Tue, 28 Aug 2018 11:49:26 +0900 (Tokyo Standard Time) Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > At Fri, 24 Aug 2018 20:44:12 +0900, Yugo Nagata <nagata@sraoss.co.jp> wrote in <20180824204412.150979ae6b283ddb639f93f6@sraoss.co.jp> > > 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 <nagata@sraoss.co.jp>
Attachment
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 textToQualifiedNameListdoes one more copy of input string more. I looked where this function is used, and I don't think soit 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
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 textToQualifiedNameListdoes one more copy of input string more. I looked where this function is used, and I don't think soit 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
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
Attachment
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
Attachment
Hi
út 9. 10. 2018 v 5:28 odesílatel Michael Paquier <michael@paquier.xyz> 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
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
Attachment
út 9. 10. 2018 v 13:20 odesílatel Michael Paquier <michael@paquier.xyz> 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
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
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
Attachment
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
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