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

From: Chao Li <[email-masked]>
Date: Mon, 17 Nov 2025 17:48:43 +0800

Hi Hackers,

== Background ==

While reviewing a patch, I noticed that quoteOneName() duplicates the logic
of quote_identifier() for adding double quotes to identifiers.  The main
difference is that quoteOneName() does *not* honor the GUC
`quote_all_identifiers`.

The typical usage pattern of quoteOneName() looks like this:

```
    // 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.

== Idea ==

I'd like to propose introducing `appendStringInfoIdentifier()`, which adds
the quoting logic directly to StringInfo.  The goals are:

* eventually deprecate quoteOneName() — I'm not aware of a reason why it
should ignore the GUC `quote_all_identifiers`
* simplify the usage patterns for quoting identifiers
* avoid unnecessary intermediate buffers, pallocs, and data copies

== About the attached patch ==

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.

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

---

From: Chao Li <[email-masked]>
Date: Thu, 20 Nov 2025 16:19:00 +0800

On Mon, Nov 17, 2025 at 5:48 PM Chao Li <[email-masked]> wrote:

> Hi Hackers,
>
> == Background ==
>
> While reviewing a patch, I noticed that quoteOneName() duplicates the
> logic of quote_identifier() for adding double quotes to identifiers.  The
> main difference is that quoteOneName() does *not* honor the GUC
> `quote_all_identifiers`.
>
> This also incurs two pallocs and two copies: quote_identifier() allocates
> a temporary buffer and copies the quoted identifier into it, and then
CF entry added: https://commitfest.postgresql.org/patch/6240/. And here
comes the v2 patch set.

Best regards,
Chao Li (Evan)
---------------------

---

From: Álvaro Herrera <[email-masked]>
Date: Thu, 20 Nov 2025 13:28:00 +0100

Hi,

> -                appendStringInfo(&buffer, _("text search configuration %s"),
> -                                 quote_qualified_identifier(nspname,
> -                                                            NameStr(cfgForm->cfgname)));
> +                appendStringInfoQualifiedIdentifier(&buffer,
> +                                                    _("text search configuration "),
> +                                                    nspname, NameStr(cfgForm->cfgname), NULL);
>                  ReleaseSysCache(tup);
>                  break;
>              }

This doesn't work from a i18n point of view.  In the original
formulation, the translator is free to place the %s wherever it suits
the language.  In the new one there's no such freedom: the name will be
appended at the end.  Some existing translations:

ko.po:msgid "text search configuration %s"
ko.po-msgstr "%s 전문 검색 구성"

tr.po:msgid "text search configuration %s"
tr.po-msgstr "%s metin arama yapılandırması"

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code.  Too bad I can't do this at work (Oracle 8/9)."       (Tom Allison)
           http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php

---

From: Chao Li <[email-masked]>
Date: Fri, 21 Nov 2025 13:52:04 +0800

> tr.po-msgstr "%s metin arama yapılandırması"

Thanks for the feedback. I reverted that piece of change in v3.

Best regards,
Chao Li (Evan)
---------------------

---

From: Chao Li <[email-masked]>
Date: Fri, 21 Nov 2025 18:03:41 +0800

Fixed a compile warning in v4.

Best regards,
Chao Li (Evan)
---------------------

---

From: Chao Li <[email-masked]>
Date: Mon, 13 Apr 2026 10:53:26 +0800

> On Nov 21, 2025, at 18:03, Chao Li <[email-masked]> wrote:
> 

Rebased.

---

From: David Rowley <[email-masked]>
Date: Mon, 13 Apr 2026 20:16:55 +1200

On Mon, 17 Nov 2025 at 22:49, Chao Li <[email-masked]> wrote:
>     quoteOneName(attname, RIAttName(pk_rel, riinfo->pk_attnums[riinfo->nkeys - 1]));
>
> 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.
>
> * 0002 switches ri_triggers.c to use it, resolving a complicated usage pattern and showing a path toward removing quoteOneName()
>

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

---

From: Chao Li <[email-masked]>
Date: Mon, 13 Apr 2026 17:28:51 +0800

> On Apr 13, 2026, at 16:16, David Rowley <[email-masked]> wrote:
> 

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,