Re: quoteOneName() inconsistency with quote_all_identifiers — replacement API proposed

> On Apr 13, 2026, at 16:16, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Mon, 17 Nov 2025 at 22:49, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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 identifier into 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 design direction.
>>
>> * 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 duplicate of quote_identifier(). However, as the implementation went on, the scope grew significantly and lost focus. In my off-list email, I mainly wanted to gauge whether you thought appendStringInfoIdentifier() was still worth pursuing, while it 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/