Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: =?utf-8?Q?Re=3A_quoteOneName=28=29_inconsistency_with_quote=5Fall?=
 =?utf-8?Q?=5Fidentifiers_=E2=80=94_replacement_API_proposed?=
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAApHDvrgHXD8myyPcgLLAjvLYC47UP0w-hU+3Ms7fOT4nksvAA@mail.gmail.com>
Date: Mon, 13 Apr 2026 17:28:51 +0800
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <07BC5357-B1DF-4BDE-8712-3D934186BF7F@gmail.com>
References: 
 <CAEoWx2=g2RVkxXB=JzWphgfg4QGV+spaA3PQ1rBM2iMehrVvjg@mail.gmail.com>
 <CAApHDvrgHXD8myyPcgLLAjvLYC47UP0w-hU+3Ms7fOT4nksvAA@mail.gmail.com>
To: David Rowley <dgrowleyml@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/07BC5357-B1DF-4BDE-8712-3D934186BF7F%40gmail.com>
Precedence: bulk


> On Apr 13, 2026, at 16:16, David Rowley <dgrowleyml@gmail.com> wrote:
>=20
> 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 =3D MAX_NAMELEN * 2 + 3 to ensure no =
overflow
>>    char attname[MAX_QUOTED_NAME_LEN];
>>=20
>>    // Add quotes and copy into the stack buffer
>>    quoteOneName(attname, RIAttName(pk_rel, =
riinfo->pk_attnums[riinfo->nkeys - 1]));
>>=20
>>    // Copy the quoted identifier into a StringInfo
>>    appendStringInfoString(&querybuf, attname);
>> ```
>>=20
>> This pattern is expensive because:
>>=20
>> * it allocates a larger-than-necessary buffer on the stack
>> * it incurs two pallocs and two data copies
>>=20
>> Looking further, the common pattern around quote_identifier() is =
similar:
>>=20
>> ```
>>    appendStringInfoString(&relations, quote_identifier(relname));
>> ```
>>=20
>> 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.
>=20
> 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?
>=20
>> Attached v1 is not intended to be the final version =E2=80=94 it is =
mainly to demonstrate the idea and get feedback on the design direction.
>>=20
>> * 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()
>>=20
>> Comments and suggestions on the overall direction would be very =
welcome.
>=20
> 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?
>=20
> 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.
>=20
> 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.
>=20
> 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:
>=20
> $ git grep -E "quote_identifier" | wc -l
> 143
>=20
> $ git grep -E "appendStringInfoString.*quote_identifier" | wc -l
> 20
>=20
> Perhaps there are more than 20, as that regex will miss ones that span
> multiple lines.
>=20
> David
>=20
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D47781

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/