Обсуждение: Fixup some appendStringInfo and appendPQExpBuffer calls
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
Вложения
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
Вложения
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)
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.)
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
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
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
Вложения
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
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
Вложения
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
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
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
Вложения
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 *.
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
Вложения
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