Add common function ReplicationOriginName.

Hi Peter,

PSA a patch to add a common function ReplicationOriginName

The patch looks good to me.

One nitpick I have is that the 2nd argument of snprintf is size_t
while we are passing int's. Your patch is consistent with the current
implementation of ReplicationOriginNameForTablesync() and similar
functions in tablesync.c. However I would like to mention this in case
the committer will be interested in replacing ints with Size's while
on it.

--
Best regards,
Aleksander Alekseev

On Mon, Sep 19, 2022 at 2:27 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:

Hi Peter,

PSA a patch to add a common function ReplicationOriginName

The patch looks good to me.

One nitpick I have is that the 2nd argument of snprintf is size_t
while we are passing int's. Your patch is consistent with the current
implementation of ReplicationOriginNameForTablesync() and similar
functions in tablesync.c.

I think it is better to use Size. Even though, it may not fail now as
the size of names for origin will always be much lesser but it is
better if we are consistent. If we agree with this, then as a first
patch, we can make it to use Size in existing places and then
introduce this new function.

--
With Regards,
Amit Kapila.

Hi Amit,

I think it is better to use Size. Even though, it may not fail now as
the size of names for origin will always be much lesser but it is
better if we are consistent. If we agree with this, then as a first
patch, we can make it to use Size in existing places and then
introduce this new function.

OK, here is the updated patchset.

* 0001 replaces int's with Size's in the existing code
* 0002 applies Peter's patch on top of 0001

--
Best regards,
Aleksander Alekseev

On Tue, Sep 20, 2022 at 6:36 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:

Hi Amit,

I think it is better to use Size. Even though, it may not fail now as
the size of names for origin will always be much lesser but it is
better if we are consistent. If we agree with this, then as a first
patch, we can make it to use Size in existing places and then
introduce this new function.

OK, here is the updated patchset.

* 0001 replaces int's with Size's in the existing code
* 0002 applies Peter's patch on top of 0001

LGTM. Thanks!

-----
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Sep 20, 2022 at 2:06 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:

Hi Amit,

I think it is better to use Size. Even though, it may not fail now as
the size of names for origin will always be much lesser but it is
better if we are consistent. If we agree with this, then as a first
patch, we can make it to use Size in existing places and then
introduce this new function.

OK, here is the updated patchset.

* 0001 replaces int's with Size's in the existing code

Pushed this one.

* 0002 applies Peter's patch on top of 0001

Can't we use the existing function ReplicationOriginNameForTablesync()
by passing relid as InvalidOid for this purpose? We need a check
inside to decide which name to construct, otherwise, it should be
fine. If we agree with this, then we can change the name of the
function to something like ReplicationOriginNameForLogicalRep or
ReplicationOriginNameForLogicalRepWorkers.

--
With Regards,
Amit Kapila.

On Wed, Sep 21, 2022 at 3:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

...

Can't we use the existing function ReplicationOriginNameForTablesync()
by passing relid as InvalidOid for this purpose? We need a check
inside to decide which name to construct, otherwise, it should be
fine. If we agree with this, then we can change the name of the
function to something like ReplicationOriginNameForLogicalRep or
ReplicationOriginNameForLogicalRepWorkers.

This suggestion attaches special meaning to the reild param.

Won't it seem a bit strange for the non-tablesync callers (who
probably have a perfectly valid 'relid') to have to pass an InvalidOid
relid just so they can format the correct origin name?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Sep 21, 2022 at 3:09 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, Sep 21, 2022 at 3:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

...

Can't we use the existing function ReplicationOriginNameForTablesync()
by passing relid as InvalidOid for this purpose? We need a check
inside to decide which name to construct, otherwise, it should be
fine. If we agree with this, then we can change the name of the
function to something like ReplicationOriginNameForLogicalRep or
ReplicationOriginNameForLogicalRepWorkers.

This suggestion attaches special meaning to the reild param.

Won't it seem a bit strange for the non-tablesync callers (who
probably have a perfectly valid 'relid') to have to pass an InvalidOid
relid just so they can format the correct origin name?

For non-tablesync workers, relid should always be InvalidOid. See, how
we launch apply workers in ApplyLauncherMain(). Do you see any case
for non-tablesync workers where relid is not InvalidOid?

--
With Regards,
Amit Kapila.

On Wed, Sep 21, 2022 at 8:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Sep 21, 2022 at 3:09 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, Sep 21, 2022 at 3:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

...

Can't we use the existing function ReplicationOriginNameForTablesync()
by passing relid as InvalidOid for this purpose? We need a check
inside to decide which name to construct, otherwise, it should be
fine. If we agree with this, then we can change the name of the
function to something like ReplicationOriginNameForLogicalRep or
ReplicationOriginNameForLogicalRepWorkers.

This suggestion attaches special meaning to the reild param.

Won't it seem a bit strange for the non-tablesync callers (who
probably have a perfectly valid 'relid') to have to pass an InvalidOid
relid just so they can format the correct origin name?

For non-tablesync workers, relid should always be InvalidOid. See, how
we launch apply workers in ApplyLauncherMain(). Do you see any case
for non-tablesync workers where relid is not InvalidOid?

Hmm, my mistake. I was thinking more of all the calls coming from the
subscriptioncmds.c, but now that I look at it maybe none of those has
any relid either.

OK, I can unify the 2 functions as you suggested. I will post another
patch in a few days.

------
Kind Regards,
Peter Smith,
Fujitsu Australia.

On Wed, Sep 21, 2022 at 8:22 PM Peter Smith <smithpb2250@gmail.com> wrote:

...

On Wed, Sep 21, 2022 at 3:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
...

Can't we use the existing function ReplicationOriginNameForTablesync()
by passing relid as InvalidOid for this purpose? We need a check
inside to decide which name to construct, otherwise, it should be
fine. If we agree with this, then we can change the name of the
function to something like ReplicationOriginNameForLogicalRep or
ReplicationOriginNameForLogicalRepWorkers.

...

OK, I can unify the 2 functions as you suggested. I will post another
patch in a few days.

PSA patch v3 to combine the different replication origin name
formatting in a single function ReplicationOriginNameForLogicalRep as
suggested.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Sep 20, 2022 at 6:36 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:

Hi Amit,

I think it is better to use Size. Even though, it may not fail now as
the size of names for origin will always be much lesser but it is
better if we are consistent. If we agree with this, then as a first
patch, we can make it to use Size in existing places and then
introduce this new function.

OK, here is the updated patchset.

* 0001 replaces int's with Size's in the existing code
* 0002 applies Peter's patch on top of 0001

Hi Aleksander.

FYI - although it is outside the scope of this thread, I did notice at
least one other example where you might want to substitute Size for
int in the same way as your v2-0001 patch did.

e.g. Just searching code for 'snprintf' where there is some parameter
for the size I quickly found:

File: src/bin/pg_dump/pg_dump_sort.c:

static void
describeDumpableObject(DumpableObject *obj, char *buf, int bufsize)

caller:
describeDumpableObject(loop[i], buf, sizeof(buf));

~~

I expect you can find more like just this if you look harder than I did.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

Hi Peter,

PSA patch v3 to combine the different replication origin name
formatting in a single function ReplicationOriginNameForLogicalRep as
suggested.

LGTM except for minor issues with the formatting; fixed.

I expect you can find more like just this if you look harder than I did.

Thanks. You are right, there are more places that pass int as the
second argument of *nprintf(). I used a command:

$ grep -r nprintf ./ | perl -lne 'print if($_ !~
/nprintf\([^\,]+,\s*(sizeof|([0-9A-Z_ \-]+\,))/ )' > found.txt

... and then re-checked the results manually. This excludes patterns
like *nprintf(..., sizeof(...)) and *nprintf(..., MACRO) and leaves
only something like *nprintf(..., variable). The cases where we
subtract an integer from a Size, etc were ignored.

I don't have a strong opinion on whether we should be really worried
by this. But in case we do, here is the patch. The order of 0001 and
0002 doesn't matter.

As I understand, ecpg uses size_t rather than Size, so for this
library I used size_t. Not 100% sure if the changes I made to
src/backend/utils/adt/jsonb.c add much value. I leave this to the
committer to decide.

--
Best regards,
Aleksander Alekseev

On Tue, Sep 27, 2022 at 5:04 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:

Hi Peter,

PSA patch v3 to combine the different replication origin name
formatting in a single function ReplicationOriginNameForLogicalRep as
suggested.

LGTM except for minor issues with the formatting; fixed.

LGTM as well. I'll push this tomorrow unless there are any more comments.

--
With Regards,
Amit Kapila.

On Mon, Oct 10, 2022 at 7:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 27, 2022 at 5:04 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:

Hi Peter,

PSA patch v3 to combine the different replication origin name
formatting in a single function ReplicationOriginNameForLogicalRep as
suggested.

LGTM except for minor issues with the formatting; fixed.

LGTM as well. I'll push this tomorrow unless there are any more comments.

Pushed.

--
With Regards,
Amit Kapila.

Hi Amit,

Pushed.

Thanks!

I don't have a strong opinion on whether we should be really worried
by this. But in case we do, here is the patch.

This leaves us one patch to deal with.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:

This leaves us one patch to deal with.
[ v4-0001-Pass-Size-size_t-as-a-2nd-argument-of-snprintf.patch ]

I looked at this and am inclined to reject it. None of these
places realistically need to deal with strings longer than
MAXPATHLEN or so, let alone multiple gigabytes. So it's just
code churn, creating backpatch hazards (admittedly not big ones)
for no real gain.

regards, tom lane

Hi Tom,

I looked at this and am inclined to reject it. [...]

OK, thanks. Then we are done with this thread. I closed the
corresponding CF entry.

--
Best regards,
Aleksander Alekseev