Re: quoteOneName() inconsistency with quote_all_identifiers — replacement API proposed - Mailing list pgsql-hackers

> On Apr 13, 2026, at 16:16, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Mon, 17 Nov 2025 at 22:49, Chao Li <li.evan.chao@gmail.com> wrote:
>> ```
>>    // Define a local buffer with size MAX_QUOTED_NAME_LEN
>>    // MAX_QUOTED_NAME_LEN = MAX_NAMELEN * 2 + 3 to ensure no overflow
>>    char attname[MAX_QUOTED_NAME_LEN];
>>
>>    // Add quotes and copy into the stack buffer
>>    quoteOneName(attname, RIAttName(pk_rel, riinfo->pk_attnums[riinfo->nkeys - 1]));
>>
>>    // Copy the quoted identifier into a StringInfo
>>    appendStringInfoString(&querybuf, attname);
>> ```
>>
>> This pattern is expensive because:
>>
>> * it allocates a larger-than-necessary buffer on the stack
>> * it incurs two pallocs and two data copies
>>
>> Looking further, the common pattern around quote_identifier() is similar:
>>
>> ```
>>    appendStringInfoString(&relations, quote_identifier(relname));
>> ```
>>
>> This also incurs two pallocs and two copies: quote_identifier() allocates a temporary buffer and copies the quoted
identifierinto it, and then appendStringInfoString() may allocate and copy again. 
>
> I'm trying to gauge where this patch is really coming from. You're
> claiming it's to improve performance, but yet it only changes a small
> subset of the code it could change. Are the ones that the v5 patches
> change only the ones that matter for performance? or are you just
> trying to improve these incrementally?
>
>> Attached v1 is not intended to be the final version — it is mainly to demonstrate the idea and get feedback on the
designdirection. 
>>
>> * 0001 implements `appendStringInfoIdentifier()` and uses it in a few places
>> * 0002 switches ri_triggers.c to use it, resolving a complicated usage pattern and showing a path toward removing
quoteOneName()
>>
>> Comments and suggestions on the overall direction would be very welcome.
>
> I don't think this is a nice design. Most of the calls to
> appendStringInfoIdentifier() have to pass NULL in one of both of the
> prefix and/or suffix. This results in some hard to read code and
> results in many extra function calls that results in the string being
> harder to read for humans and harder to grep for. I think even if you
> had a nice design, there'd be a huge amount of churn to fully
> implement it. Do you have some sort of evidence as performance numbers
> that this is worthwhile churn?
>
> To acknowledge your off-list email pinging me about this thread and
> referencing my recent commits in the area of StringInfo; to be clear,
> those only change code that's new to or was changed in v19, and
> they're all following a pattern that was agreed on many years ago. The
> reason for doing those post-freeze is that we don't yet have a better
> way to identify them sooner, and, per historical evidence of periodic
> fixes, we expect these would eventually get fixed, and doing that
> before we branch means there are fewer backpatching conflicts for
> committers than there would be if we waited until after branching.
>
> Looking around for better ideas for you... Going by the latest in [1],
> there's been no progress getting custom format specifier checking
> added to GCC, so I guess that means we don't want to improve this with
> a custom format specifier.
>
> This is just my view, so feel free to get someone else's, but I think
> if you want to make improvements here, then you'll need to come up
> with a design that's clearly better than what we have, otherwise the
> churn is just not worthwhile. I don't have any great ideas on what you
> could do. I see going by the following there are 20
> appendStringInfoString() calls that use quote_identifier() on the
> string argument. Those could be improved with a new function in
> stringinfo.c that handles that, but that only gets you about 14% of
> the way there, going by:
>
> $ git grep -E "quote_identifier" | wc -l
> 143
>
> $ git grep -E "appendStringInfoString.*quote_identifier" | wc -l
> 20
>
> Perhaps there are more than 20, as that regex will miss ones that span
> multiple lines.
>
> David
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47781

Thanks for the detailed explanation.

This is a fairly old patch, and the initial motivation was simply to deprecate quoteOneName(), as it seemed like a
duplicateof quote_identifier(). However, as the implementation went on, the scope grew significantly and lost focus. In
myoff-list email, I mainly wanted to gauge whether you thought appendStringInfoIdentifier() was still worth pursuing,
whileit avoids a memory allocation and copy, I also had doubts regarding the design of the prefix and suffix
parameters.

I think the answer is clear now. I'm going to withdraw this patch.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/