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

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