Horton
Menu

Thread

  1. quoteOneName() inconsistency with quote_all_identifiers — replacement API proposed

    Chao Li <li.evan.chao@gmail.com> — 2025-11-17T09:48:43Z

    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/

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-20T08:19:00Z

    On Mon, Nov 17, 2025 at 5:48 PM Chao Li <li.evan.chao@gmail.com> 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`.
>
> 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.
>
>
CF entry added: https://commitfest.postgresql.org/patch/6240/. And here
comes the v2 patch set.

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

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-20T12:28:00Z

    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

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-21T05:52:04Z

    On Thu, Nov 20, 2025 at 8:28 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> 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ı"
>

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

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

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-21T10:03:41Z

    Fixed a compile warning in v4.

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


On Fri, Nov 21, 2025 at 1:52 PM Chao Li <li.evan.chao@gmail.com> wrote:

>
> On Thu, Nov 20, 2025 at 8:28 PM Álvaro Herrera <alvherre@kurilemu.de>
> wrote:
>
>> 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ı"
>>
>
> Thanks for the feedback. I reverted that piece of change in v3.
>
> Best regards,
> Chao Li (Evan)
> ---------------------
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>

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

    Chao Li <li.evan.chao@gmail.com> — 2026-04-13T02:53:26Z

    
> On Nov 21, 2025, at 18:03, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> Fixed a compile warning in v4.
> 
> Best regards,
> Chao Li (Evan)
> ---------------------
> HighGo Software Co., Ltd.
> https://www.highgo.com/
> 
> 
> On Fri, Nov 21, 2025 at 1:52 PM Chao Li <li.evan.chao@gmail.com> wrote:
> 
> On Thu, Nov 20, 2025 at 8:28 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 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ı"
> 
> Thanks for the feedback. I reverted that piece of change in v3.
> 
> Best regards,
> Chao Li (Evan)
> ---------------------
> HighGo Software Co., Ltd.
> https://www.highgo.com/ 

Rebased.

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

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

    David Rowley <dgrowleyml@gmail.com> — 2026-04-13T08:16:55Z

    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 = 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

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

    Chao Li <li.evan.chao@gmail.com> — 2026-04-13T09:28:51Z

    
> On Apr 13, 2026, at 16:16, David Rowley <dgrowleyml@gmail.com> wrote:
> 
> 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 = 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/