Horton

Thread

Commits

  1. Clean up some questionable usages of DatumGet* macros

  2. Standardize usages of appendStringInfo and appendPQExpBuffer

  1. Fixup some appendStringInfo and appendPQExpBuffer calls

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-06-02T01:37:51Z

    Hi,

In the latest HEAD branch, I found some places were using
appendStringInfo/appendPQExpBuffer() when they could have been using
appendStringInfoString/ appendPQExpBufferStr() instead. I think we'd better
fix these places in case other developers will use these codes as a reference,
though, it seems will not bring noticeable performance gain.

Attaching a patch to fix these places.

Best regards,
houzj

  2. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Michael Paquier <michael@paquier.xyz> — 2021-06-02T04:29:26Z

    On Wed, Jun 02, 2021 at 01:37:51AM +0000, houzj.fnst@fujitsu.com wrote:
> In the latest HEAD branch, I found some places were using
> appendStringInfo/appendPQExpBuffer() when they could have been using
> appendStringInfoString/ appendPQExpBufferStr() instead. I think we'd better
> fix these places in case other developers will use these codes as a reference,
> though, it seems will not bring noticeable performance gain.

Indeed, that's the same thing as 110d817 to make all those calls
cheaper.  No objections from me to do those changes now rather than
later on HEAD.
--
Michael

  3. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-02T10:57:01Z

    On 2021-Jun-02, houzj.fnst@fujitsu.com wrote:

> Hi,
> 
> In the latest HEAD branch, I found some places were using
> appendStringInfo/appendPQExpBuffer() when they could have been using
> appendStringInfoString/ appendPQExpBufferStr() instead. I think we'd better
> fix these places in case other developers will use these codes as a reference,
> though, it seems will not bring noticeable performance gain.

hmm why didn't we get warnings about the PENDING DETACH one?  Maybe we
need some decorator in PQExpBuffer.


-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"La espina, desde que nace, ya pincha" (Proverbio africano)

  4. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-06-02T14:55:49Z

    On 02.06.21 12:57, Alvaro Herrera wrote:
>> In the latest HEAD branch, I found some places were using
>> appendStringInfo/appendPQExpBuffer() when they could have been using
>> appendStringInfoString/ appendPQExpBufferStr() instead. I think we'd better
>> fix these places in case other developers will use these codes as a reference,
>> though, it seems will not bring noticeable performance gain.
> 
> hmm why didn't we get warnings about the PENDING DETACH one?  Maybe we
> need some decorator in PQExpBuffer.

I don't think there is anything wrong with the existing code there. 
It's just like using printf() when you could use puts().

(I'm not against the proposed patch, just answering this question.)

  5. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    David Rowley <dgrowleyml@gmail.com> — 2021-06-03T01:53:34Z

    On Wed, 2 Jun 2021 at 16:29, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jun 02, 2021 at 01:37:51AM +0000, houzj.fnst@fujitsu.com wrote:
> > In the latest HEAD branch, I found some places were using
> > appendStringInfo/appendPQExpBuffer() when they could have been using
> > appendStringInfoString/ appendPQExpBufferStr() instead. I think we'd better
> > fix these places in case other developers will use these codes as a reference,
> > though, it seems will not bring noticeable performance gain.
>
> Indeed, that's the same thing as 110d817 to make all those calls
> cheaper.  No objections from me to do those changes now rather than
> later on HEAD.

I think it would be good to fix at least the instances that are new
code in PG14 before we branch for PG15.  They all seem low enough risk
and worth keeping the new-to-PG14 code as close to the same as
possible between major versions. It seems more likely that newer code
will need bug fixes in the future so having the code as similar as
possible in each branch makes backpatching easier.

For the code that's not new to PG14, I feel less strongly about those.
In the patch there's just 2 instances of these; one in
contrib/sepgsql/schema.c and another in
src/backend/postmaster/postmaster.c.  I've tried to push for these
sorts of things to be fixed at around this time of year in the past,
but there have been other people thinking we should wait until we
branch. For example [1] and [2].

David

[1] https://www.postgresql.org/message-id/CAKJS1f9APLTZRomOSndx_nFcFNfUxncz%3Dp2_-1wr0hrzT4ELKg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/4a84839e-afe4-ea27-6823-23372511dcbf%402ndquadrant.com

  6. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-03T02:56:38Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Wed, 2 Jun 2021 at 16:29, Michael Paquier <michael@paquier.xyz> wrote:
>> Indeed, that's the same thing as 110d817 to make all those calls
>> cheaper.  No objections from me to do those changes now rather than
>> later on HEAD.

> I think it would be good to fix at least the instances that are new
> code in PG14 before we branch for PG15.  They all seem low enough risk
> and worth keeping the new-to-PG14 code as close to the same as
> possible between major versions.

+1 for fixing this sort of thing in new code before we branch.

I'm less interested in changing code that already exists in back
branches.  I think the risk of causing headaches for back-patches
may outweigh any benefit of such micro-optimizations.

			regards, tom lane

  7. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Michael Paquier <michael@paquier.xyz> — 2021-06-03T03:00:56Z

    On Thu, Jun 03, 2021 at 01:53:34PM +1200, David Rowley wrote:
> I think it would be good to fix at least the instances that are new
> code in PG14 before we branch for PG15.  They all seem low enough risk
> and worth keeping the new-to-PG14 code as close to the same as
> possible between major versions. It seems more likely that newer code
> will need bug fixes in the future so having the code as similar as
> possible in each branch makes backpatching easier.

> For the code that's not new to PG14, I feel less strongly about those.
> In the patch there's just 2 instances of these; one in
> contrib/sepgsql/schema.c and another in
> src/backend/postmaster/postmaster.c.  I've tried to push for these
> sorts of things to be fixed at around this time of year in the past,
> but there have been other people thinking we should wait until we
> branch. For example [1] and [2].

No objections to those arguments, makes sense.  I don't see an issue
with changing the new code before branching, FWIW.  As you already did
110d817, perhaps you would prefer taking care of it?
--
Michael

  8. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    David Rowley <dgrowleyml@gmail.com> — 2021-06-03T03:06:57Z

    On Thu, 3 Jun 2021 at 15:01, Michael Paquier <michael@paquier.xyz> wrote:
> As you already did
> 110d817, perhaps you would prefer taking care of it?

Ok. I'll take care of it.

Thanks

David

  9. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    David Rowley <dgrowleyml@gmail.com> — 2021-06-03T03:51:48Z

    On Thu, 3 Jun 2021 at 15:06, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Thu, 3 Jun 2021 at 15:01, Michael Paquier <michael@paquier.xyz> wrote:
> > As you already did
> > 110d817, perhaps you would prefer taking care of it?
>
> Ok. I'll take care of it.

I looked at this and couldn't help but notice how the following used
DatumGetPointer() instead of DatumGetCString():

appendStringInfo(&str, "%s ... %s",
                 DatumGetPointer(a),
                 DatumGetPointer(b));

However, looking a bit further it looks like instead of using
FunctionCall1 to call the type's output function, that the code should
use OutputFunctionCall and get a char * directly.

e.g the attached.

There are quite a few other places in that file that should be using
DatumGetCString() instead of DatumGetPointer().

Should we fix those too for PG14?

In the meantime, I'll push a version of this with just the StringInfo
fixes first. If we do anything else it can be done as a separate
commit.

David

  10. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-03T04:17:06Z

    David Rowley <dgrowleyml@gmail.com> writes:
> There are quite a few other places in that file that should be using
> DatumGetCString() instead of DatumGetPointer().
> Should we fix those too for PG14?

+1.  I'm surprised we are not getting compiler warnings.

			regards, tom lane

  11. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    David Rowley <dgrowleyml@gmail.com> — 2021-06-03T04:39:06Z

    On Thu, 3 Jun 2021 at 15:06, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Thu, 3 Jun 2021 at 15:01, Michael Paquier <michael@paquier.xyz> wrote:
> > As you already did
> > 110d817, perhaps you would prefer taking care of it?
>
> Ok. I'll take care of it.

Pushed.

David

  12. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    David Rowley <dgrowleyml@gmail.com> — 2021-06-03T06:51:46Z

    On Thu, 3 Jun 2021 at 16:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > There are quite a few other places in that file that should be using
> > DatumGetCString() instead of DatumGetPointer().
> > Should we fix those too for PG14?
>
> +1.  I'm surprised we are not getting compiler warnings.

I've attached a patch to fix those.

I did end up getting in a little deeper than I'd have liked as I also
found a few typos along the way.

Also, going by my calendar, the copyright year was incorrect.

Tomas, any chance you could look over this?  I didn't really take the
time to understand the code, so some of my comment adjustments might
be incorrect.

David

  13. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-06-03T13:03:14Z

    On 03.06.21 06:17, Tom Lane wrote:
> David Rowley <dgrowleyml@gmail.com> writes:
>> There are quite a few other places in that file that should be using
>> DatumGetCString() instead of DatumGetPointer().
>> Should we fix those too for PG14?
> 
> +1.  I'm surprised we are not getting compiler warnings.

Well, DatumGetPointer() returns Pointer, and Pointer is char *.

  14. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    David Rowley <dgrowleyml@gmail.com> — 2021-06-04T10:53:58Z

    On Thu, 3 Jun 2021 at 16:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > There are quite a few other places in that file that should be using
> > DatumGetCString() instead of DatumGetPointer().
> > Should we fix those too for PG14?
>
> +1.  I'm surprised we are not getting compiler warnings.

I pushed a fix for this.

I did happen to find one other in mcv.c which dates back to 2019. I
was wondering if we should bother with that one since it's already out
there in PG13.

David

  15. Re: Fixup some appendStringInfo and appendPQExpBuffer calls

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-04T13:30:53Z

    David Rowley <dgrowleyml@gmail.com> writes:
> I did happen to find one other in mcv.c which dates back to 2019. I
> was wondering if we should bother with that one since it's already out
> there in PG13.

Maybe not.  Per Peter's point, it's just cosmetic really.

			regards, tom lane