public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Chao Li <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: quoteOneName() inconsistency with quote_all_identifiers — replacement API proposed
Date: Mon, 13 Apr 2026 20:16:55 +1200
Message-ID: <CAApHDvrgHXD8myyPcgLLAjvLYC47UP0w-hU+3Ms7fOT4nksvAA@mail.gmail.com> (raw)
In-Reply-To: <CAEoWx2=g2RVkxXB=JzWphgfg4QGV+spaA3PQ1rBM2iMehrVvjg@mail.gmail.com>
References: <CAEoWx2=g2RVkxXB=JzWphgfg4QGV+spaA3PQ1rBM2iMehrVvjg@mail.gmail.com>
On Mon, 17 Nov 2025 at 22:49, Chao Li <[email protected]> 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
view thread (8+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Re: quoteOneName() inconsistency with quote_all_identifiers — replacement API proposed
In-Reply-To: <CAApHDvrgHXD8myyPcgLLAjvLYC47UP0w-hU+3Ms7fOT4nksvAA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox