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
