Daniel Gustafsson <dan...@yesql.se> writes:
> On 23 Sep 2020, at 21:33, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> 2. In the case where encoding conversion is performed, we still have
>> to pstrdup the result to have a safe copy for "curline". But I
>> realized that we're making a poor choice of which copy to return to
>> the caller. The output of encoding conversion is likely to be a good
>> bit bigger than necessary, cf. pg_do_encoding_conversion. So if the
>> caller is one that saves the output string directly into a long-lived
>> dictionary structure, this wastes space. We should return the pstrdup
>> result instead, and keep the conversion result as "curline", where
>> we'll free it next time through.
> I wonder if we have other callsites of pg_any_to_server which could benefit
> from lowering the returned allocation, a quick look didn't spot any but today
> has become yesterday here and tiredness might interfere.
After looking more closely, I've realized that actually none of the
existing core-code callers will save the returned string directly.
readstoplist() could do so, depending on what "wordop" is, but
all its existing callers pass lowerstr() which will always make a
new output string. (Which itself could be a bit bloated :-()
So the concern I expressed above is really just hypothetical.
Still, the code is simpler this way and no slower, so I still
think it's an improvement.
(The bigger picture here is that the API for dictionary init
methods is pretty seriously misdesigned from a memory-consumption
standpoint. Running the entire init process in the dictionary's
long-lived context goes against everything we've learned about
avoiding memory leaks. We should run that code in a short-lived
command execution context, and explicitly copy just the data we
want into the long-lived context. But changing that would be
a pretty big deal, breaking third-party dictionaries. So I'm
not sure it's enough of a problem to justify the change.)
regards, tom lane
