Fixup some appendStringInfo and appendPQExpBuffer calls

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]/messages/by-id/CAKJS1f9APLTZRomOSndx_nFcFNfUxncz=p2_-1wr0hrzT4ELKg@mail.gmail.com and [2]/messages/by-id/4a84839e-afe4-ea27-6823-23372511dcbf@2ndquadrant.com.

David

[1]: /messages/by-id/CAKJS1f9APLTZRomOSndx_nFcFNfUxncz=p2_-1wr0hrzT4ELKg@mail.gmail.com
[2]: /messages/by-id/4a84839e-afe4-ea27-6823-23372511dcbf@2ndquadrant.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