Thread: speed up a logical replica setup

Hi,

On 2022-02-21 09:09:12 -0300, Euler Taveira wrote:
> Logical replication has been used to migration with minimal downtime. However,
> if you are dealing with a big database, the amount of required resources (disk
> -- due to WAL retention) increases as the backlog (WAL) increases. Unless you
> have a generous amount of resources and can wait for long period of time until
> the new replica catches up, creating a logical replica is impracticable on
> large databases.

Indeed.


> DESIGN
> 
> The conversion requires 8 steps.
> 
> 1. Check if the target data directory has the same system identifier than the
> source data directory.
> 2. Stop the target server if it is running as a standby server. (Modify
> recovery parameters requires a restart.)
> 3. Create one replication slot per specified database on the source server. One
> additional replication slot is created at the end to get the consistent LSN
> (This consistent LSN will be used as (a) a stopping point for the recovery
> process and (b) a starting point for the subscriptions).
> 4. Write recovery parameters into the target data directory and start the
> target server (Wait until the target server is promoted).
> 5. Create one publication (FOR ALL TABLES) per specified database on the source
> server.
> 6. Create one subscription per specified database on the target server (Use
> replication slot and publication created in a previous step. Don't enable the
> subscriptions yet).
> 7. Sets the replication progress to the consistent LSN that was got in a
> previous step.
> 8. Enable the subscription for each specified database on the target server.

I think the system identifier should also be changed, otherwise you can way
too easily get into situations trying to apply WAL from different systems to
each other. Not going to end well, obviously.


> This tool does not take a base backup. It can certainly be included later.
> There is already a tool do it: pg_basebackup.

It would make sense to allow to call pg_basebackup from the new tool. Perhaps
with a --pg-basebackup-parameters or such.



Greetings,

Andres Freund

I think the system identifier should also be changed, otherwise you can way

too easily get into situations trying to apply WAL from different systems to

each other. Not going to end well, obviously.

> This tool does not take a base backup. It can certainly be included later.

> There is already a tool do it: pg_basebackup.

It would make sense to allow to call pg_basebackup from the new tool. Perhaps

with a --pg-basebackup-parameters or such.

On Mon, Feb 21, 2022 at 5:41 PM Euler Taveira <euler@eulerto.com> wrote:
>
> Logical replication has been used to migration with minimal downtime. However,
> if you are dealing with a big database, the amount of required resources (disk
> -- due to WAL retention) increases as the backlog (WAL) increases. Unless you
> have a generous amount of resources and can wait for long period of time until
> the new replica catches up, creating a logical replica is impracticable on
> large databases.
>
> The general idea is to create and convert a physical replica or a base backup
> (archived WAL files available) into a logical replica. The initial data copy
> and catchup tends to be faster on a physical replica. This technique has been
> successfully used in pglogical_create_subscriber [1].
>

Sounds like a promising idea.

> A new tool called pg_subscriber does this conversion and is tightly integrated
> with Postgres.
>
> DESIGN
>
> The conversion requires 8 steps.
>
> 1. Check if the target data directory has the same system identifier than the
> source data directory.
> 2. Stop the target server if it is running as a standby server. (Modify
> recovery parameters requires a restart.)
> 3. Create one replication slot per specified database on the source server. One
> additional replication slot is created at the end to get the consistent LSN
> (This consistent LSN will be used as (a) a stopping point for the recovery
> process and (b) a starting point for the subscriptions).
>

What is the need to create an extra slot other than the slot for each
database? Can't we use the largest LSN returned by slots as the
recovery-target-lsn and starting point for subscriptions?

How, these additional slots will get freed or reused when say the
server has crashed/stopped after creating the slots but before
creating the subscriptions? Users won't even know the names of such
slots as they are internally created.

> 4. Write recovery parameters into the target data directory and start the
> target server (Wait until the target server is promoted).
> 5. Create one publication (FOR ALL TABLES) per specified database on the source
> server.
> 6. Create one subscription per specified database on the target server (Use
> replication slot and publication created in a previous step. Don't enable the
> subscriptions yet).
> 7. Sets the replication progress to the consistent LSN that was got in a
> previous step.
> 8. Enable the subscription for each specified database on the target server.
>
> This tool does not take a base backup. It can certainly be included later.
> There is already a tool do it: pg_basebackup.
>

The backup will take the backup of all the databases present on the
source server. Do we need to provide the way/recommendation to remove
the databases that are not required?

Can we see some numbers with various sizes of databases (cluster) to
see how it impacts the time for small to large size databases as
compared to the traditional method? This might help giving users
advice on when to use this tool?


-- 
With Regards,
Amit Kapila.

On 2/21/22 13:09, Euler Taveira wrote:
> DESIGN
> 
> The conversion requires 8 steps.
> 
> 1. Check if the target data directory has the same system identifier 
> than the
> source data directory.
> 2. Stop the target server if it is running as a standby server. (Modify
> recovery parameters requires a restart.)
> 3. Create one replication slot per specified database on the source 
> server. One
> additional replication slot is created at the end to get the consistent LSN
> (This consistent LSN will be used as (a) a stopping point for the recovery
> process and (b) a starting point for the subscriptions).
> 4. Write recovery parameters into the target data directory and start the
> target server (Wait until the target server is promoted).
> 5. Create one publication (FOR ALL TABLES) per specified database on the 
> source
> server.
> 6. Create one subscription per specified database on the target server (Use
> replication slot and publication created in a previous step. Don't 
> enable the
> subscriptions yet).
> 7. Sets the replication progress to the consistent LSN that was got in a
> previous step.
> 8. Enable the subscription for each specified database on the target server.

Very interesting!

I actually just a couple of weeks ago proposed a similar design for 
upgrading a database of a customer of mine. We have not tried it yet so 
it is not decided if we should go ahead with it.

In our case the goal is a bit different so my idea is that we will use 
pg_dump/pg_restore (or pg_upgrade and then some manual cleanup if 
pg_dump/pg_restore is too slow) on the target server. The goal of this 
design is to get a nice clean logical replica at the new version of 
PostgreSQL with indexes with the correct collations, all old invalid 
constraints validated, minimal bloat, etc. And all of this without 
creating bloat or putting too much load on the old master during the 
process. We have plenty of disk space and plenty of time so those are 
not limitations in our case. I can go into more detail if there is interest.

It is nice to see that our approach is not entirely unique. :) And I 
will take a look at this patch when I find the time.

Andreas

On 21.02.22 13:09, Euler Taveira wrote:
> A new tool called pg_subscriber does this conversion and is tightly 
> integrated
> with Postgres.

Are we comfortable with the name pg_subscriber?  It seems too general.
Are we planning other subscriber-related operations in the future?  If
so, we should at least make this one use a --create option or
something like that.

  doc/src/sgml/ref/pg_subscriber.sgml

Attached is a patch that reorganizes the man page a bit.  I moved the
description of the steps to the Notes section and formatted it
differently.  I think the steps are interesting but not essential for
the using of the program, so I wanted to get them out of the main
description.

  src/bin/pg_subscriber/pg_subscriber.c

+   if (made_temp_replslot)
+   {
+       conn = connect_database(dbinfo[0].pubconninfo, true);
+       drop_replication_slot(conn, &dbinfo[0], temp_replslot);
+       disconnect_database(conn);
+   }

Temp slots don't need to be cleaned up.

+/*
+ * Obtain the system identifier from control file. It will be used to 
compare
+ * if a data directory is a clone of another one. This routine is used 
locally
+ * and avoids a replication connection.
+ */
+static char *
+get_control_from_datadir(const char *datadir)

This could return uint64 directly, without string conversion.
get_sysid_from_conn() could then convert to uint64 internally.

+       {"verbose", no_argument, NULL, 'v'},

I'm not sure if the --verbose option is all that useful.

+       {"stop-subscriber", no_argument, NULL, 1},

This option doesn't seem to be otherwise supported or documented.

+   pub_sysid = pg_malloc(32);
+   pub_sysid = get_sysid_from_conn(dbinfo[0].pubconninfo);
+   sub_sysid = pg_malloc(32);
+   sub_sysid = get_control_from_datadir(subscriber_dir);

These mallocs don't appears to be of any use.

+   dbname_conninfo = pg_malloc(NAMEDATALEN);

This seems wrong.

Overall, this code could use a little bit more structuring.  There are
a lot of helper functions that don't seem to do a lot and are mostly
duplicate runs-this-SQL-command calls.  But the main() function is
still huge.  There is room for refinement.

  src/bin/pg_subscriber/t/001_basic.pl

Good start, but obviously, we'll need some real test cases here also.

  src/bin/initdb/initdb.c
  src/bin/pg_ctl/pg_ctl.c
  src/common/file_utils.c
  src/include/common/file_utils.h

I recommend skipping this refactoring.  The readfile() function from
pg_ctl is not general enough to warrant the pride of place of a
globally available function.  Note that it is specifically geared
toward some of pg_ctl's requirements, for example that the underlying
file can change while it is being read.

The requirements of pg_subscriber can be satisfied more easily: Just
call pg_ctl to start the server.  You are already using that in
pg_subscriber.  Is there a reason it can't be used here as well?

On 3/15/22 09:51, Peter Eisentraut wrote:
> On 21.02.22 13:09, Euler Taveira wrote:
>> A new tool called pg_subscriber does this conversion and is tightly
>> integrated
>> with Postgres.
>
> Are we comfortable with the name pg_subscriber?  It seems too general.
> Are we planning other subscriber-related operations in the future?  If
> so, we should at least make this one use a --create option or
> something like that.


Not really sold on the name (and I didn't much like the name
pglogical_create_subscriber either, although it's a cool facility and
I'm happy to see us adopting something like it).

ISTM we should have a name that conveys that we are *converting* a
replica or equivalent to a subscriber.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2022-02-21 09:09:12 -0300, Euler Taveira wrote:
> A new tool called pg_subscriber does this conversion and is tightly integrated
> with Postgres.

Given that this has been submitted just before the last CF and is a patch of
nontrivial size, has't made significant progress ISTM it should be moved to
the next CF?

It currently fails in cfbot, but that's likely just due to Peter's incremental
patch. Perhaps you could make sure the patch still applies and repost?

Greetings,

Andres Freund

On 18.03.22 23:34, Andrew Dunstan wrote:
> On 3/15/22 09:51, Peter Eisentraut wrote:
>> On 21.02.22 13:09, Euler Taveira wrote:
>>> A new tool called pg_subscriber does this conversion and is tightly
>>> integrated
>>> with Postgres.
>>
>> Are we comfortable with the name pg_subscriber?  It seems too general.
>> Are we planning other subscriber-related operations in the future?  If
>> so, we should at least make this one use a --create option or
>> something like that.
> 
> 
> Not really sold on the name (and I didn't much like the name
> pglogical_create_subscriber either, although it's a cool facility and
> I'm happy to see us adopting something like it).
> 
> ISTM we should have a name that conveys that we are *converting* a
> replica or equivalent to a subscriber.

The pglogical tool includes the pg_basebackup run, so it actually 
"creates" the subscriber from scratch.  Whether this tool is also doing 
that is still being discussed.

On 22.03.22 02:25, Andres Freund wrote:
> On 2022-02-21 09:09:12 -0300, Euler Taveira wrote:
>> A new tool called pg_subscriber does this conversion and is tightly integrated
>> with Postgres.
> 
> Given that this has been submitted just before the last CF and is a patch of
> nontrivial size, has't made significant progress ISTM it should be moved to
> the next CF?

done

This entry has been waiting on author input for a while (our current
threshold is roughly two weeks), so I've marked it Returned with
Feedback.

Once you think the patchset is ready for review again, you (or any
interested party) can resurrect the patch entry by visiting

    https://commitfest.postgresql.org/38/3556/

and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)

Thanks,
--Jacob

A new tool called pg_subscriber does this conversion and is tightly integrated

with Postgres.

On Mon, Oct 23, 2023 at 9:34 AM Euler Taveira <euler@eulerto.com> wrote:

>
> It is still a WIP but I would like to share it and get some feedback.
>
>

I have started reviewing the patch. I have just read through all the
code. It's well documented and clear. Next I will review the design in
detail. Here are a couple of minor comments
1.
+tests += {
+ 'name': 'pg_subscriber',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+ 'tests': [
+ 't/001_basic.pl',

COMMENT
Shouldn't we include 002_standby.pl?

2. CreateReplicationSlotLSN, is not used anywhere. Instead I see
create_logical_replication_slot() in pg_subscriber.c. Which of these
two you intend to use finally?

--
Best Wishes,
Ashutosh Bapat

I think this is duplicate with https://commitfest.postgresql.org/45/4637/

The new status of this patch is: Waiting on Author

On Thu, Oct 26, 2023 at 5:17 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Mon, Oct 23, 2023 at 9:34 AM Euler Taveira <euler@eulerto.com> wrote:
>
> >
> > It is still a WIP but I would like to share it and get some feedback.
> >
> >
>
> I have started reviewing the patch. I have just read through all the
> code. It's well documented and clear. Next I will review the design in
> detail.

Here are some comments about functionality and design.

+ <step>
+ <para>
+ <application>pg_subscriber</application> creates one replication slot for
+ each specified database on the source server. The replication slot name
+ contains a <literal>pg_subscriber</literal> prefix. These replication
+ slots will be used by the subscriptions in a future step. Another
+ replication slot is used to get a consistent start location. This
+ consistent LSN will be used as a stopping point in the <xref
+ linkend="guc-recovery-target-lsn"/> parameter and by the
+ subscriptions as a replication starting point. It guarantees that no
+ transaction will be lost.
+ </para>
+ </step>

CREATE_REPLICATION_SLOT would wait for any incomplete transaction to
complete. So it may not be possible to have an incomplete transaction
on standby when it comes out of recovery. Am I correct? Can we please
have a testcase where we test this scenario? What about a prepared
transactions?

+
+ <step>
+ <para>
+ <application>pg_subscriber</application> writes recovery parameters into
+ the target data directory and start the target server. It specifies a LSN
+ (consistent LSN that was obtained in the previous step) of write-ahead
+ log location up to which recovery will proceed. It also specifies
+ <literal>promote</literal> as the action that the server should take once
+ the recovery target is reached. This step finishes once the server ends
+ standby mode and is accepting read-write operations.
+ </para>
+ </step>

At this stage the standby would have various replication objects like
publications, subscriptions, origins inherited from the upstream
server and possibly very much active. With failover slots, it might
inherit replication slots. Is it intended that the new subscriber also
acts as publisher for source's subscribers OR that the new subscriber
should subscribe to the upstreams of the source? Some use cases like
logical standby might require that but a multi-master multi-node setup
may not. The behaviour should be user configurable.

There may be other objects in this category which need special consideration on
the subscriber. I haven't fully thought through the list of such objects.

+ uses the replication slot that was created in a previous step. The
+ subscription is created but it is not enabled yet. The reason is the
+ replication progress must be set to the consistent LSN but replication
+ origin name contains the subscription oid in its name. Hence, the

Not able to understand the sentence "The reason is ... in its name".
Why is subscription OID in origin name matters?

+ <para>
+ <application>pg_subscriber</application> stops the target server to change
+ its system identifier.
+ </para>

I expected the subscriber to be started after this step.

Why do we need pg_resetwal?

+ appendPQExpBuffer(str, "CREATE_REPLICATION_SLOT \"%s\"", slot_name);
+ appendPQExpBufferStr(str, " LOGICAL \"pgoutput\" NOEXPORT_SNAPSHOT");

Hardcoding output plugin name would limit this utility only to
built-in plugin. Any reason for that limitation?

In its current form the utility creates a logical subscriber which
subscribes to all the tables (and sequences when we have sequence
replication). But it will be useful even in case of selective
replication from a very large database. In such a case the new
subscriber will need to a. remove the unwanted objects b. subscriber
will need to subscribe to publications publishing the "interesting"
objects. We don't need to support this case, but the current
functionality (including the interface) and design shouldn't limit us
from doing so. Have you thought about this case?

I noticed some differences between this and a similar utility
https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_create_subscriber.c.
I will be reviewing these differences next to see if we are missing
anything here.

--
Best Wishes,
Ashutosh Bapat

I think this is duplicate with https://commitfest.postgresql.org/45/4637/

The new status of this patch is: Waiting on Author

On Wed, Nov 1, 2023 at 7:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> I noticed some differences between this and a similar utility
> https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_create_subscriber.c.
> I will be reviewing these differences next to see if we are missing
> anything here.

Some more missing things to discuss

Handling signals - The utility cleans up left over objects on exit.
But default signal handlers will make the utility exit without a
proper cleanup [1]. The signal handlers may clean up the objects
themselves or at least report the objects that need tobe cleaned up.

Idempotent behaviour - Given that the utility will be used when very
large amount of data is involved, redoing everything after a network
glitch or a temporary failure should be avoided. This is true when the
users start with base backup. Again, I don't think we should try to be
idempotent in v1 but current design shouldn't stop us from doing so. I
didn't find anything like that in my review. But something to keep in
mind.

That finishes my first round of review. I will wait for your updated
patches before the next round.

[1] NOTEs section in man atexit().


--
Best Wishes,
Ashutosh Bapat

On 23.10.23 05:53, Euler Taveira wrote:
> Let's continue with the bike shedding... I agree with Peter E that this name
> does not express what this tool is. At the moment, it only have one action:
> create. If I have to suggest other actions I would say that it could support
> switchover option too (that removes the infrastructure created by this 
> tool).
> If we decide to keep this name, it should be a good idea to add an option to
> indicate what action it is executing (similar to pg_recvlogical) as 
> suggested
> by Peter.

Speaking of which, would it make sense to put this tool (whatever the 
name) into the pg_basebackup directory?  It's sort of related, and it 
also shares some code.

On Tue, Nov 07, 2023 at 10:00:39PM +0100, Peter Eisentraut wrote:
> Speaking of which, would it make sense to put this tool (whatever the name)
> into the pg_basebackup directory?  It's sort of related, and it also shares
> some code.

I've read the patch, and the additions to streamutil.h and
streamutil.c make it kind of natural to have it sit in pg_basebackup/.
There's pg_recvlogical already there.  I am wondering about two
things, though:
- Should the subdirectory pg_basebackup be renamed into something more
generic at this point?  All these things are frontend tools that deal
in some way with the replication protocol to do their work.  Say
a replication_tools?
- And if it would be better to refactor some of the code generic to
all these streaming tools to fe_utils.  What makes streamutil.h a bit
less pluggable are all its extern variables to control the connection,
but perhaps that can be an advantage, as well, in some cases.
--
Michael

On Tue, Nov 07, 2023 at 10:00:39PM +0100, Peter Eisentraut wrote:

> Speaking of which, would it make sense to put this tool (whatever the name)

> into the pg_basebackup directory?  It's sort of related, and it also shares

> some code.

I've read the patch, and the additions to streamutil.h and

streamutil.c make it kind of natural to have it sit in pg_basebackup/.

There's pg_recvlogical already there.  I am wondering about two

things, though:

- Should the subdirectory pg_basebackup be renamed into something more

generic at this point?  All these things are frontend tools that deal

in some way with the replication protocol to do their work.  Say

a replication_tools?

- And if it would be better to refactor some of the code generic to

all these streaming tools to fe_utils.  What makes streamutil.h a bit

less pluggable are all its extern variables to control the connection,

but perhaps that can be an advantage, as well, in some cases.

On Wed, Nov 08, 2023 at 09:50:47AM -0300, Euler Taveira wrote:
> On Tue, Nov 7, 2023, at 8:12 PM, Michael Paquier wrote:
> I used the CreateReplicationSlot() from streamutil.h but decided to use the
> CREATE_REPLICATION_SLOT command directly because it needs the LSN as output. As
> you noticed at that time I wouldn't like a dependency in the pg_basebackup
> header files; if we move this binary to base backup directory, it seems natural
> to refactor the referred function and use it.

Right.  That should be OK to store that in an optional XLogRecPtr
pointer, aka by letting the option to pass NULL as argument of the
function if the caller needs nothing.

>> I've read the patch, and the additions to streamutil.h and
>> streamutil.c make it kind of natural to have it sit in pg_basebackup/.
>> There's pg_recvlogical already there.  I am wondering about two
>> things, though:
>> - Should the subdirectory pg_basebackup be renamed into something more
>> generic at this point?  All these things are frontend tools that deal
>> in some way with the replication protocol to do their work.  Say
>> a replication_tools?
>
> It is a good fit for this tool since it is another replication tool. I also
> agree with the directory renaming; it seems confusing that the directory has
> the same name as one binary but also contains other related binaries in it.

Or cluster_tools?  Or stream_tools?  replication_tools may be OK, but
I have a bad sense in naming new things around here.  So if anybody
has a better idea, feel free..

>> - And if it would be better to refactor some of the code generic to
>> all these streaming tools to fe_utils.  What makes streamutil.h a bit
>> less pluggable are all its extern variables to control the connection,
>> but perhaps that can be an advantage, as well, in some cases.
>
> I like it. There are common functions such as GetConnection(),
> CreateReplicationSlot(), DropReplicationSlot() and RunIdentifySystem() that is
> used by all of these replication tools. We can move the extern variables into
> parameters to have a pluggable streamutil.h.

And perhaps RetrieveWalSegSize() as well as GetSlotInformation().
These kick replication commands.
--
Michael

On 08.11.23 00:12, Michael Paquier wrote:
> - Should the subdirectory pg_basebackup be renamed into something more
> generic at this point?  All these things are frontend tools that deal
> in some way with the replication protocol to do their work.  Say
> a replication_tools?

Seems like unnecessary churn.  Nobody has complained about any of the 
other tools in there.

> - And if it would be better to refactor some of the code generic to
> all these streaming tools to fe_utils.  What makes streamutil.h a bit
> less pluggable are all its extern variables to control the connection,
> but perhaps that can be an advantage, as well, in some cases.

Does anyone outside of pg_basebackup + existing friends + new friend 
need that?  Seems like extra complications.

On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> On 08.11.23 00:12, Michael Paquier wrote:
>> - Should the subdirectory pg_basebackup be renamed into something more
>> generic at this point?  All these things are frontend tools that deal
>> in some way with the replication protocol to do their work.  Say
>> a replication_tools?
>
> Seems like unnecessary churn.  Nobody has complained about any of the other
> tools in there.

Not sure.  We rename things across releases in the tree from time to
time, and here that's straight-forward.

>> - And if it would be better to refactor some of the code generic to
>> all these streaming tools to fe_utils.  What makes streamutil.h a bit
>> less pluggable are all its extern variables to control the connection,
>> but perhaps that can be an advantage, as well, in some cases.
>
> Does anyone outside of pg_basebackup + existing friends + new friend need
> that?  Seems like extra complications.

Actually, yes, I've used these utility routines in some past work, and
having the wrapper routines able to run the replication commands in
fe_utils would have been nicer than having to link to a source tree.
--
Michael

On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:

> On 08.11.23 00:12, Michael Paquier wrote:

>> - Should the subdirectory pg_basebackup be renamed into something more

>> generic at this point?  All these things are frontend tools that deal

>> in some way with the replication protocol to do their work.  Say

>> a replication_tools?

> 

> Seems like unnecessary churn.  Nobody has complained about any of the other

> tools in there.

Not sure.  We rename things across releases in the tree from time to

time, and here that's straight-forward.

Hi,

On Wed, 6 Dec 2023 at 12:53, Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> > On 08.11.23 00:12, Michael Paquier wrote:
> >> - Should the subdirectory pg_basebackup be renamed into something more
> >> generic at this point?  All these things are frontend tools that deal
> >> in some way with the replication protocol to do their work.  Say
> >> a replication_tools?
> >
> > Seems like unnecessary churn.  Nobody has complained about any of the other
> > tools in there.
>
> Not sure.  We rename things across releases in the tree from time to
> time, and here that's straight-forward.
>
>
> Based on this discussion it seems we have a consensus that this tool should be
> in the pg_basebackup directory. (If/when we agree with the directory renaming,
> it could be done in a separate patch.) Besides this move, the v3 provides a dry
> run mode. It basically executes every routine but skip when should do
> modifications. It is an useful option to check if you will be able to run it
> without having issues with connectivity, permission, and existing objects
> (replication slots, publications, subscriptions). Tests were slightly improved.
> Messages were changed to *not* provide INFO messages by default and --verbose
> provides INFO messages and --verbose --verbose also provides DEBUG messages. I
> also refactored the connect_database() function into which the connection will
> always use the logical replication mode. A bug was fixed in the transient
> replication slot name. Ashutosh review [1] was included. The code was also indented.
>
> There are a few suggestions from Ashutosh [2] that I will reply in another
> email.
>
> I'm still planning to work on the following points:
>
> 1. improve the cleanup routine to point out leftover objects if there is any
>    connection issue.
> 2. remove the physical replication slot if the standby is using one
>    (primary_slot_name).
> 3. provide instructions to promote the logical replica into primary, I mean,
>    stop the replication between the nodes and remove the replication setup
>    (publications, subscriptions, replication slots). Or even include another
>    action to do it. We could add both too.
>
> Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice
> UI for users that would like to use it.
>
>
> [1] https://postgr.es/m/CAExHW5sCAU3NvPKd7msScQKvrBN-x_AdDQD-ZYAwOxuWG%3Doz1w%40mail.gmail.com
> [2] https://postgr.es/m/CAExHW5vHFemFvTUHe+7XWphVZJxrEXz5H3dD4UQi7CwmdMJQYg@mail.gmail.com
>

The changes in the file 'src/tools/msvc/Mkvcbuild.pm' seems
unnecessary as the folder 'msvc' is removed due to the commit [1].


To review the changes, I did 'git reset --hard' to the commit previous
to commit [1].
I tried to build the postgres on my Windows machine using two methods:
i. building using Visual Studio
ii. building using Meson

When I built the code using Visual Studio, on installing postgres,
pg_subscriber binary was not created.
But when I built the code using Meson, on installing postgres,
pg_subscriber binary was created.
Is this behaviour intentional?

[1] https://github.com/postgres/postgres/commit/1301c80b2167feb658a738fa4ceb1c23d0991e23

Thanks and Regards,
Shlok Kyal

When I built the code using Visual Studio, on installing postgres,

pg_subscriber binary was not created.

But when I built the code using Meson, on installing postgres,

pg_subscriber binary was created.

Is this behaviour intentional?

On Wed, Dec 6, 2023 at 12:53 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> > On 08.11.23 00:12, Michael Paquier wrote:
> >> - Should the subdirectory pg_basebackup be renamed into something more
> >> generic at this point?  All these things are frontend tools that deal
> >> in some way with the replication protocol to do their work.  Say
> >> a replication_tools?
> >
> > Seems like unnecessary churn.  Nobody has complained about any of the other
> > tools in there.
>
> Not sure.  We rename things across releases in the tree from time to
> time, and here that's straight-forward.
>
>
> Based on this discussion it seems we have a consensus that this tool should be
> in the pg_basebackup directory. (If/when we agree with the directory renaming,
> it could be done in a separate patch.) Besides this move, the v3 provides a dry
> run mode. It basically executes every routine but skip when should do
> modifications. It is an useful option to check if you will be able to run it
> without having issues with connectivity, permission, and existing objects
> (replication slots, publications, subscriptions). Tests were slightly improved.
> Messages were changed to *not* provide INFO messages by default and --verbose
> provides INFO messages and --verbose --verbose also provides DEBUG messages. I
> also refactored the connect_database() function into which the connection will
> always use the logical replication mode. A bug was fixed in the transient
> replication slot name. Ashutosh review [1] was included. The code was also indented.
>
> There are a few suggestions from Ashutosh [2] that I will reply in another
> email.
>
> I'm still planning to work on the following points:
>
> 1. improve the cleanup routine to point out leftover objects if there is any
>    connection issue.
>

I think this is an important part. Shall we try to write to some file
the pending objects to be cleaned up? We do something like that during
the upgrade.

> 2. remove the physical replication slot if the standby is using one
>    (primary_slot_name).
> 3. provide instructions to promote the logical replica into primary, I mean,
>    stop the replication between the nodes and remove the replication setup
>    (publications, subscriptions, replication slots). Or even include another
>    action to do it. We could add both too.
>
> Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice
> UI for users that would like to use it.
>

Isn't point 2 also essential because how would otherwise such a slot
be advanced or removed?

A few other points:
==============
1. Previously, I asked whether we need an additional replication slot
patch created to get consistent LSN and I see the following comment in
the patch:

+ *
+ * XXX we should probably use the last created replication slot to get a
+ * consistent LSN but it should be changed after adding pg_basebackup
+ * support.

Yeah, sure, we may want to do that after backup support and we can
keep a comment for the same but I feel as the patch stands today,
there is no good reason to keep it. Also, is there a reason that we
can't create the slots after backup is complete and before we write
recovery parameters

2.
+ appendPQExpBuffer(str,
+   "CREATE SUBSCRIPTION %s CONNECTION '%s' PUBLICATION %s "
+   "WITH (create_slot = false, copy_data = false, enabled = false)",
+   dbinfo->subname, dbinfo->pubconninfo, dbinfo->pubname);

Shouldn't we enable two_phase by default for newly created
subscriptions? Is there a reason for not doing so?

3. How about sync slots on the physical standby if present? Do we want
to retain those as it is or do we need to remove those? We are
actively working on the patch [1] for the same.

4. Can we see some numbers with various sizes of databases (cluster)
to see how it impacts the time for small to large-size databases as
compared to the traditional method? This might help us with giving
users advice on when to use this tool. We can do this bit later as
well when the patch is closer to being ready for commit.

[1] -
https://www.postgresql.org/message-id/OS0PR01MB5716DAF72265388A2AD424119495A%40OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.

On Wed, Nov 1, 2023 at 7:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Here are some comments about functionality and design.
>
> + <step>
> + <para>
> + <application>pg_subscriber</application> creates one replication slot for
> + each specified database on the source server. The replication slot name
> + contains a <literal>pg_subscriber</literal> prefix. These replication
> + slots will be used by the subscriptions in a future step. Another
> + replication slot is used to get a consistent start location. This
> + consistent LSN will be used as a stopping point in the <xref
> + linkend="guc-recovery-target-lsn"/> parameter and by the
> + subscriptions as a replication starting point. It guarantees that no
> + transaction will be lost.
> + </para>
> + </step>
>
> CREATE_REPLICATION_SLOT would wait for any incomplete transaction to
> complete. So it may not be possible to have an incomplete transaction
> on standby when it comes out of recovery. Am I correct? Can we please
> have a testcase where we test this scenario? What about a prepared
> transactions?
>

It will wait even for prepared transactions to commit. So, there
shouldn't be any behavior difference for prepared and non-prepared
transactions.

> +
> + <step>
> + <para>
> + <application>pg_subscriber</application> writes recovery parameters into
> + the target data directory and start the target server. It specifies a LSN
> + (consistent LSN that was obtained in the previous step) of write-ahead
> + log location up to which recovery will proceed. It also specifies
> + <literal>promote</literal> as the action that the server should take once
> + the recovery target is reached. This step finishes once the server ends
> + standby mode and is accepting read-write operations.
> + </para>
> + </step>
>
> At this stage the standby would have various replication objects like
> publications, subscriptions, origins inherited from the upstream
> server and possibly very much active. With failover slots, it might
> inherit replication slots. Is it intended that the new subscriber also
> acts as publisher for source's subscribers OR that the new subscriber
> should subscribe to the upstreams of the source? Some use cases like
> logical standby might require that but a multi-master multi-node setup
> may not. The behaviour should be user configurable.
>

Good points but even if we make it user configurable how to exclude
such replication objects? And if we don't exclude then what will be
their use because if one wants to use it as a logical standby then we
only need publications and failover/sync slots in it and also there
won't be a need to create new slots, publications on the primary to
make the current physical standby as logical subscriber.

> There may be other objects in this category which need special consideration on
> the subscriber. I haven't fully thought through the list of such objects.
>
> + uses the replication slot that was created in a previous step. The
> + subscription is created but it is not enabled yet. The reason is the
> + replication progress must be set to the consistent LSN but replication
> + origin name contains the subscription oid in its name. Hence, the
>
> Not able to understand the sentence "The reason is ... in its name".
> Why is subscription OID in origin name matters?
>

Using subscription OID in origin is probably to uniquely identify the
origin corresponding to the subscription, we do that while creating a
subscription as well.

--
With Regards,
Amit Kapila.

On Wed, 6 Dec 2023 at 12:53, Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> > On 08.11.23 00:12, Michael Paquier wrote:
> >> - Should the subdirectory pg_basebackup be renamed into something more
> >> generic at this point?  All these things are frontend tools that deal
> >> in some way with the replication protocol to do their work.  Say
> >> a replication_tools?
> >
> > Seems like unnecessary churn.  Nobody has complained about any of the other
> > tools in there.
>
> Not sure.  We rename things across releases in the tree from time to
> time, and here that's straight-forward.
>
>
> Based on this discussion it seems we have a consensus that this tool should be
> in the pg_basebackup directory. (If/when we agree with the directory renaming,
> it could be done in a separate patch.) Besides this move, the v3 provides a dry
> run mode. It basically executes every routine but skip when should do
> modifications. It is an useful option to check if you will be able to run it
> without having issues with connectivity, permission, and existing objects
> (replication slots, publications, subscriptions). Tests were slightly improved.
> Messages were changed to *not* provide INFO messages by default and --verbose
> provides INFO messages and --verbose --verbose also provides DEBUG messages. I
> also refactored the connect_database() function into which the connection will
> always use the logical replication mode. A bug was fixed in the transient
> replication slot name. Ashutosh review [1] was included. The code was also indented.
>
> There are a few suggestions from Ashutosh [2] that I will reply in another
> email.
>
> I'm still planning to work on the following points:
>
> 1. improve the cleanup routine to point out leftover objects if there is any
>    connection issue.
> 2. remove the physical replication slot if the standby is using one
>    (primary_slot_name).
> 3. provide instructions to promote the logical replica into primary, I mean,
>    stop the replication between the nodes and remove the replication setup
>    (publications, subscriptions, replication slots). Or even include another
>    action to do it. We could add both too.
>
> Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice
> UI for users that would like to use it.

1) This Assert can fail if source is shutdown:
+static void
+drop_replication_slot(PGconn *conn, LogicalRepInfo *dbinfo, const
char *slot_name)
+{
+       PQExpBuffer str = createPQExpBuffer();
+       PGresult   *res;
+
+       Assert(conn != NULL);

I could simulate it by shutting the primary while trying to reach the
consistent state:
pg_subscriber: postmaster reached the consistent state
pg_subscriber: error: connection to database failed: connection to
server at "localhost" (127.0.0.1), port 5432 failed: Connection
refused
Is the server running on that host and accepting TCP/IP connections?
pg_subscriber: error: connection to database failed: connection to
server at "localhost" (127.0.0.1), port 5432 failed: Connection
refused
Is the server running on that host and accepting TCP/IP connections?
pg_subscriber: error: connection to database failed: connection to
server at "localhost" (127.0.0.1), port 5432 failed: Connection
refused
Is the server running on that host and accepting TCP/IP connections?
pg_subscriber: pg_subscriber.c:692: drop_replication_slot: Assertion
`conn != ((void *)0)' failed.
Aborted

2) Should we have some checks to see if the max replication slot
configuration is ok based on the number of slots that will be created,
we have similar checks in upgrade replication slots in
check_new_cluster_logical_replication_slots

3) Should we check if wal_level is set to logical, we have similar
checks in upgrade replication slots in
check_new_cluster_logical_replication_slots

4) The physical replication slot that was created will still be
present in the primary node, I felt this should be removed.

5) I felt the target server should be started before completion of
pg_subscriber:
+       /*
+        * Stop the subscriber.
+        */
+       pg_log_info("stopping the subscriber");
+
+       pg_ctl_cmd = psprintf("\"%s\" stop -D \"%s\" -s", pg_ctl_path,
subscriber_dir);
+       rc = system(pg_ctl_cmd);
+       pg_ctl_status(pg_ctl_cmd, rc, 0);
+
+       /*
+        * Change system identifier.
+        */
+       modify_sysid(pg_resetwal_path, subscriber_dir);
+
+       success = true;
+
+       pg_log_info("Done!");
+
+       return 0;

Regards,
Vignesh

On Wed, 1 Nov 2023 at 19:28, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> At this stage the standby would have various replication objects like
> publications, subscriptions, origins inherited from the upstream
> server and possibly very much active. With failover slots, it might
> inherit replication slots. Is it intended that the new subscriber also
> acts as publisher for source's subscribers OR that the new subscriber
> should subscribe to the upstreams of the source? Some use cases like
> logical standby might require that but a multi-master multi-node setup
> may not. The behaviour should be user configurable.

How about we do like this:
a) Starting the server in binary upgrade mode(so that the existing
subscriptions will not try to connect to the publishers) b) Disable
the subscriptions c) Drop the replication slots d) Drop the
publications e) Then restart the server in normal(non-upgrade) mode.
f) The rest of pg_subscriber work like
create_all_logical_replication_slots, create_subscription,
set_replication_progress, enable_subscription, etc
This will be done by default. There will be an option
--clean-logical-replication-info provided to allow DBA not to clean
the objects if DBA does not want to remove these objects.
I felt cleaning the replication information is better as a) Node-1
will replicate all the data to Node-2 (that Node-1 is subscribing to
from other nodes) after pg_subscriber setup is done. b) all the data
that Node-1 is publishing need not be published again by Node-2. There
is an option to override if the user does not want to remove the
logical replication objects.

Regards,
Vignesh

On Wed, Jan 3, 2024 at 12:09 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 1 Nov 2023 at 19:28, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > At this stage the standby would have various replication objects like
> > publications, subscriptions, origins inherited from the upstream
> > server and possibly very much active. With failover slots, it might
> > inherit replication slots. Is it intended that the new subscriber also
> > acts as publisher for source's subscribers OR that the new subscriber
> > should subscribe to the upstreams of the source? Some use cases like
> > logical standby might require that but a multi-master multi-node setup
> > may not. The behaviour should be user configurable.
>
> How about we do like this:
> a) Starting the server in binary upgrade mode(so that the existing
> subscriptions will not try to connect to the publishers)
>

Can't we simply do it by starting the server with
max_logical_replication_workers = 0 or is there some other need to
start in binary upgrade mode?

 b) Disable
> the subscriptions
>

Why not simply drop the subscriptions?

 c) Drop the replication slots d) Drop the
> publications
>

I am not so sure about dropping publications because, unlike
subscriptions which can start to pull the data, there is no harm with
publications. Similar to publications there could be some user-defined
functions or other other objects which may not be required once the
standby replica is converted to subscriber. I guess we need to leave
those to the user.

 e) Then restart the server in normal(non-upgrade) mode.
> f) The rest of pg_subscriber work like
> create_all_logical_replication_slots, create_subscription,
> set_replication_progress, enable_subscription, etc
> This will be done by default. There will be an option
> --clean-logical-replication-info provided to allow DBA not to clean
> the objects if DBA does not want to remove these objects.
>

I agree that some kind of switch to control this action would be useful.

--
With Regards,
Amit Kapila.

On Wed, 3 Jan 2024 at 14:49, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jan 3, 2024 at 12:09 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, 1 Nov 2023 at 19:28, Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > At this stage the standby would have various replication objects like
> > > publications, subscriptions, origins inherited from the upstream
> > > server and possibly very much active. With failover slots, it might
> > > inherit replication slots. Is it intended that the new subscriber also
> > > acts as publisher for source's subscribers OR that the new subscriber
> > > should subscribe to the upstreams of the source? Some use cases like
> > > logical standby might require that but a multi-master multi-node setup
> > > may not. The behaviour should be user configurable.
> >
> > How about we do like this:
> > a) Starting the server in binary upgrade mode(so that the existing
> > subscriptions will not try to connect to the publishers)
> >
>
> Can't we simply do it by starting the server with
> max_logical_replication_workers = 0 or is there some other need to
> start in binary upgrade mode?

I agree, max_logical_replication_workers = 0 is enough for our case.

>  b) Disable
> > the subscriptions
> >
>
> Why not simply drop the subscriptions?

Dropping subscriptions is ok as these subscriptions will not be required.

>  c) Drop the replication slots d) Drop the
> > publications
> >
>
> I am not so sure about dropping publications because, unlike
> subscriptions which can start to pull the data, there is no harm with
> publications. Similar to publications there could be some user-defined
> functions or other other objects which may not be required once the
> standby replica is converted to subscriber. I guess we need to leave
> those to the user.

Yes, that makes sense.

Regards,
Vignesh

I think this is an important part. Shall we try to write to some file

the pending objects to be cleaned up? We do something like that during

the upgrade.

> 2. remove the physical replication slot if the standby is using one

>    (primary_slot_name).

> 3. provide instructions to promote the logical replica into primary, I mean,

>    stop the replication between the nodes and remove the replication setup

>    (publications, subscriptions, replication slots). Or even include another

>    action to do it. We could add both too.

>

> Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice

> UI for users that would like to use it.

>

Isn't point 2 also essential because how would otherwise such a slot

be advanced or removed?

A few other points:

==============

1. Previously, I asked whether we need an additional replication slot

patch created to get consistent LSN and I see the following comment in

the patch:

+ *

+ * XXX we should probably use the last created replication slot to get a

+ * consistent LSN but it should be changed after adding pg_basebackup

+ * support.

Yeah, sure, we may want to do that after backup support and we can

keep a comment for the same but I feel as the patch stands today,

there is no good reason to keep it.

Also, is there a reason that we

can't create the slots after backup is complete and before we write

recovery parameters

2.

+ appendPQExpBuffer(str,

+   "CREATE SUBSCRIPTION %s CONNECTION '%s' PUBLICATION %s "

+   "WITH (create_slot = false, copy_data = false, enabled = false)",

+   dbinfo->subname, dbinfo->pubconninfo, dbinfo->pubname);

Shouldn't we enable two_phase by default for newly created

subscriptions? Is there a reason for not doing so?

3. How about sync slots on the physical standby if present? Do we want

to retain those as it is or do we need to remove those? We are

actively working on the patch [1] for the same.

4. Can we see some numbers with various sizes of databases (cluster)

to see how it impacts the time for small to large-size databases as

compared to the traditional method? This might help us with giving

users advice on when to use this tool. We can do this bit later as

well when the patch is closer to being ready for commit.

1) This Assert can fail if source is shutdown:

+static void

+drop_replication_slot(PGconn *conn, LogicalRepInfo *dbinfo, const

char *slot_name)

+{

+       PQExpBuffer str = createPQExpBuffer();

+       PGresult   *res;

+

+       Assert(conn != NULL);

2) Should we have some checks to see if the max replication slot

configuration is ok based on the number of slots that will be created,

we have similar checks in upgrade replication slots in

check_new_cluster_logical_replication_slots

3) Should we check if wal_level is set to logical, we have similar

checks in upgrade replication slots in

check_new_cluster_logical_replication_slots

4) The physical replication slot that was created will still be

present in the primary node, I felt this should be removed.

5) I felt the target server should be started before completion of

pg_subscriber:

On Thu, Jan 4, 2024 at 8:24 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Dec 21, 2023, at 3:16 AM, Amit Kapila wrote:
>
> > 2. remove the physical replication slot if the standby is using one
> >    (primary_slot_name).
> > 3. provide instructions to promote the logical replica into primary, I mean,
> >    stop the replication between the nodes and remove the replication setup
> >    (publications, subscriptions, replication slots). Or even include another
> >    action to do it. We could add both too.
> >
> > Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice
> > UI for users that would like to use it.
> >
>
> Isn't point 2 also essential because how would otherwise such a slot
> be advanced or removed?
>
>
> I'm worried about a scenario that you will still use the primary. (Let's say
> the logical replica will be promoted to a staging or dev server.) No connection
> between primary and this new server so the primary slot is useless after the
> promotion.
>

So, you also seem to be saying that it is not required once
pg_subscriber has promoted it. So, why it should be optional to remove
physical_replication_slot? I think we must remove it from the primary
unless there is some other reason.

> A few other points:
> ==============
> 1. Previously, I asked whether we need an additional replication slot
> patch created to get consistent LSN and I see the following comment in
> the patch:
>
> + *
> + * XXX we should probably use the last created replication slot to get a
> + * consistent LSN but it should be changed after adding pg_basebackup
> + * support.
>
> Yeah, sure, we may want to do that after backup support and we can
> keep a comment for the same but I feel as the patch stands today,
> there is no good reason to keep it.
>
>
> I'll remove the comment to avoid confusing.
>

My point is to not have an additional slot and keep a comment
indicating that we need an extra slot once we add pg_basebackup
support.

>
> 2.
> + appendPQExpBuffer(str,
> +   "CREATE SUBSCRIPTION %s CONNECTION '%s' PUBLICATION %s "
> +   "WITH (create_slot = false, copy_data = false, enabled = false)",
> +   dbinfo->subname, dbinfo->pubconninfo, dbinfo->pubname);
>
> Shouldn't we enable two_phase by default for newly created
> subscriptions? Is there a reason for not doing so?
>
>
> Why? I decided to keep the default for some settings (streaming,
> synchronous_commit, two_phase, disable_on_error). Unless there is a compelling
> reason to enable it, I think we should use the default. Either way, data will
> arrive on subscriber as soon as the prepared transaction is committed.
>

I thought we could provide a better experience for logical replicas
created by default but I see your point and probably keeping default
values for parameters you mentioned seems reasonable to me.

> 3. How about sync slots on the physical standby if present? Do we want
> to retain those as it is or do we need to remove those? We are
> actively working on the patch [1] for the same.
>
>
> I didn't read the current version of the referred patch but if the proposal is
> to synchronize logical replication slots iif you are using a physical
> replication, as soon as pg_subscriber finishes the execution, there won't be
> synchronization on these logical replication slots because there isn't a
> physical replication anymore. If the goal is a promotion, the current behavior
> is correct because the logical replica will retain WAL since it was converted.
>

I don't understand what you mean by promotion in this context. If
users want to simply promote the standby then there is no need to do
additional things that this tool is doing.

> However, if you are creating a logical replica, this WAL retention is not good
> and the customer should eventually remove these logical replication slots on
> the logical replica.
>

I think asking users to manually remove such slots won't be a good
idea. We might want to either remove them by default or provide an
option to the user.

--
With Regards,
Amit Kapila.

On Thu, Jan 4, 2024 at 8:52 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Mon, Jan 1, 2024, at 7:14 AM, vignesh C wrote:
>
>
> 5) I felt the target server should be started before completion of
> pg_subscriber:
>
>
> Why?
>

Won't it be a better user experience that after setting up the target
server as a logical replica (subscriber), it started to work
seamlessly without user intervention?

> The initial version had an option to stop the subscriber. I decided to
> remove the option and stop the subscriber by default mainly because (1) it is
> an extra step to start the server (another point is that the WAL retention
> doesn't happen due to additional (synchronized?) replication slots on
> subscriber -- point 2). It was a conservative choice. If point 2 isn't an
> issue, imo point 1 is no big deal.
>

By point 2, do you mean to have a check for "max replication slots"?
It so, the one possibility is to even increase that config, if the
required max_replication_slots is low.

--
With Regards,
Amit Kapila.

Hi,
I was testing the patch with following test cases:

Test 1 :
- Create a 'primary' node
- Setup physical replica using pg_basebackup  "./pg_basebackup –h
localhost –X stream –v –R –W –D ../standby "
- Insert data before and after pg_basebackup
- Run pg_subscriber and then insert some data to check logical
replication "./pg_subscriber –D ../standby -S “host=localhost
port=9000 dbname=postgres” -P “host=localhost port=9000
dbname=postgres” -d postgres"
- Also check pg_publication, pg_subscriber and pg_replication_slots tables.

Observation:
Data is not lost. Replication is happening correctly. Pg_subscriber is
working as expected.

Test 2:
- Create a 'primary' node
- Use normal pg_basebackup but don’t set up Physical replication
"./pg_basebackup –h localhost –v –W –D ../standby"
- Insert data before and after pg_basebackup
- Run pg_subscriber

Observation:
Pg_subscriber command is not completing and is stuck with following
log repeating:
LOG: waiting for WAL to become available at 0/3000168
LOG: invalid record length at 0/3000150: expected at least 24, got 0

Test 3:
- Create a 'primary' node
- Use normal pg_basebackup but don’t set up Physical replication
"./pg_basebackup –h localhost –v –W –D ../standby"
-Insert data before pg_basebackup but not after pg_basebackup
-Run pg_subscriber

Observation:
Pg_subscriber command is not completing and is stuck with following
log repeating:
LOG: waiting for WAL to become available at 0/3000168
LOG: invalid record length at 0/3000150: expected at least 24, got 0

I was not clear about how to use pg_basebackup in this case, can you
let me know if any changes need to be made for test2 and test3.

Thanks and regards
Shlok Kyal

On Wed, Jan 3, 2024 at 2:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>  c) Drop the replication slots d) Drop the
> > publications
> >
>
> I am not so sure about dropping publications because, unlike
> subscriptions which can start to pull the data, there is no harm with
> publications. Similar to publications there could be some user-defined
> functions or other other objects which may not be required once the
> standby replica is converted to subscriber. I guess we need to leave
> those to the user.
>

IIUC, primary use of pg_subscriber utility is to start a logical
subscription from a physical base backup (to reduce initial sync time)
as against logical backup taken while creating a subscription. Hence I
am expecting that apart from this difference, the resultant logical
replica should look similar to the logical replica setup using a
logical subscription sync. Hence we should not leave any replication
objects around. UDFs (views, and other objects) may have some use on a
logical replica. We may replicate changes to UDF once DDL replication
is supported. But what good is having the same publications as primary
also on logical replica?

--
Best Wishes,
Ashutosh Bapat

On Thu, Jan 4, 2024 at 12:30 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Jan 3, 2024 at 2:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >  c) Drop the replication slots d) Drop the
> > > publications
> > >
> >
> > I am not so sure about dropping publications because, unlike
> > subscriptions which can start to pull the data, there is no harm with
> > publications. Similar to publications there could be some user-defined
> > functions or other other objects which may not be required once the
> > standby replica is converted to subscriber. I guess we need to leave
> > those to the user.
> >
>
> IIUC, primary use of pg_subscriber utility is to start a logical
> subscription from a physical base backup (to reduce initial sync time)
> as against logical backup taken while creating a subscription. Hence I
> am expecting that apart from this difference, the resultant logical
> replica should look similar to the logical replica setup using a
> logical subscription sync. Hence we should not leave any replication
> objects around. UDFs (views, and other objects) may have some use on a
> logical replica. We may replicate changes to UDF once DDL replication
> is supported. But what good is having the same publications as primary
> also on logical replica?
>

The one use case that comes to my mind is to set up bi-directional
replication. The publishers want to subscribe to the new subscriber.

--
With Regards,
Amit Kapila.

On Thu, Jan 4, 2024 at 12:22 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> Hi,
> I was testing the patch with following test cases:
>
> Test 1 :
> - Create a 'primary' node
> - Setup physical replica using pg_basebackup  "./pg_basebackup –h
> localhost –X stream –v –R –W –D ../standby "
> - Insert data before and after pg_basebackup
> - Run pg_subscriber and then insert some data to check logical
> replication "./pg_subscriber –D ../standby -S “host=localhost
> port=9000 dbname=postgres” -P “host=localhost port=9000
> dbname=postgres” -d postgres"
> - Also check pg_publication, pg_subscriber and pg_replication_slots tables.
>
> Observation:
> Data is not lost. Replication is happening correctly. Pg_subscriber is
> working as expected.
>
> Test 2:
> - Create a 'primary' node
> - Use normal pg_basebackup but don’t set up Physical replication
> "./pg_basebackup –h localhost –v –W –D ../standby"
> - Insert data before and after pg_basebackup
> - Run pg_subscriber
>
> Observation:
> Pg_subscriber command is not completing and is stuck with following
> log repeating:
> LOG: waiting for WAL to become available at 0/3000168
> LOG: invalid record length at 0/3000150: expected at least 24, got 0
>

I think probably the required WAL is not copied. Can you use the -X
option to stream WAL as well and then test? But I feel in this case
also, we should wait for some threshold time and then exit with
failure, removing new objects created, if any.

> Test 3:
> - Create a 'primary' node
> - Use normal pg_basebackup but don’t set up Physical replication
> "./pg_basebackup –h localhost –v –W –D ../standby"
> -Insert data before pg_basebackup but not after pg_basebackup
> -Run pg_subscriber
>
> Observation:
> Pg_subscriber command is not completing and is stuck with following
> log repeating:
> LOG: waiting for WAL to become available at 0/3000168
> LOG: invalid record length at 0/3000150: expected at least 24, got 0
>

This is similar to the previous test and you can try the same option
here as well.

--
With Regards,
Amit Kapila.

On Thu, Jan 4, 2024 at 4:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > But what good is having the same publications as primary
> > also on logical replica?
> >
>
> The one use case that comes to my mind is to set up bi-directional
> replication. The publishers want to subscribe to the new subscriber.

Hmm. Looks like another user controlled cleanup.

--
Best Wishes,
Ashutosh Bapat

So, you also seem to be saying that it is not required once

pg_subscriber has promoted it. So, why it should be optional to remove

physical_replication_slot? I think we must remove it from the primary

unless there is some other reason.

My point is to not have an additional slot and keep a comment

indicating that we need an extra slot once we add pg_basebackup

support.

> 3. How about sync slots on the physical standby if present? Do we want

> to retain those as it is or do we need to remove those? We are

> actively working on the patch [1] for the same.

>

>

> I didn't read the current version of the referred patch but if the proposal is

> to synchronize logical replication slots iif you are using a physical

> replication, as soon as pg_subscriber finishes the execution, there won't be

> synchronization on these logical replication slots because there isn't a

> physical replication anymore. If the goal is a promotion, the current behavior

> is correct because the logical replica will retain WAL since it was converted.

>

I don't understand what you mean by promotion in this context. If

users want to simply promote the standby then there is no need to do

additional things that this tool is doing.

> However, if you are creating a logical replica, this WAL retention is not good

> and the customer should eventually remove these logical replication slots on

> the logical replica.

>

I think asking users to manually remove such slots won't be a good

idea. We might want to either remove them by default or provide an

option to the user.

Won't it be a better user experience that after setting up the target

server as a logical replica (subscriber), it started to work

seamlessly without user intervention?

> The initial version had an option to stop the subscriber. I decided to

> remove the option and stop the subscriber by default mainly because (1) it is

> an extra step to start the server (another point is that the WAL retention

> doesn't happen due to additional (synchronized?) replication slots on

> subscriber -- point 2). It was a conservative choice. If point 2 isn't an

> issue, imo point 1 is no big deal.

>

By point 2, do you mean to have a check for "max replication slots"?

It so, the one possibility is to even increase that config, if the

required max_replication_slots is low.

On Thu, Jan 4, 2024 at 9:18 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Jan 4, 2024, at 2:41 AM, Amit Kapila wrote:
>
>
> I think asking users to manually remove such slots won't be a good
> idea. We might want to either remove them by default or provide an
> option to the user.
>
>
> Am I correct that the majority of the use cases these replication slots will be
> useless?
>

I am not so sure about it. Say, if some sync slots are present this
means the user wants this replica to be used later as a publisher.
Now, if the existing primary/publisher node is still alive then we
don't have these slots but if the user wants to switch over to this
new node as well then they may be required.

Is there a possibility that a cascading standby also has a slot on the
current physical replica being converted to a new subscriber?

> If so, let's remove them by default and add an option to control this
> behavior (replication slot removal).
>

The presence of slots on the physical replica indicates that the other
nodes/clusters could be dependent on it, so, I feel by default we
should give an error and if the user uses some option to remove slots
then it is fine to remove them.

--
With Regards,
Amit Kapila.

On Thu, Jan 4, 2024 at 9:27 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Jan 4, 2024, at 3:05 AM, Amit Kapila wrote:
>
> Won't it be a better user experience that after setting up the target
> server as a logical replica (subscriber), it started to work
> seamlessly without user intervention?
>
>
> If we have an option to control the replication slot removal (default is on),
> it seems a good UI. Even if the user decides to disable the replication slot
> removal, it should print a message saying that these replication slots can
> cause WAL retention.
>

As pointed out in the previous response, I think we should not proceed
with such a risk of WAL retention and other nodes dependency, we
should either give an ERROR (default) or remove slots, if the user
provides an option. If we do so, do you think by default we can keep
the server started or let the user start it later? I think one
advantage of letting the user start it later is that she gets a chance
to adjust config parameters in postgresql.conf and by default we won't
be using system resources.

--
With Regards,
Amit Kapila.

On Thu, 4 Jan 2024 at 16:46, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jan 4, 2024 at 12:22 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > Hi,
> > I was testing the patch with following test cases:
> >
> > Test 1 :
> > - Create a 'primary' node
> > - Setup physical replica using pg_basebackup  "./pg_basebackup –h
> > localhost –X stream –v –R –W –D ../standby "
> > - Insert data before and after pg_basebackup
> > - Run pg_subscriber and then insert some data to check logical
> > replication "./pg_subscriber –D ../standby -S “host=localhost
> > port=9000 dbname=postgres” -P “host=localhost port=9000
> > dbname=postgres” -d postgres"
> > - Also check pg_publication, pg_subscriber and pg_replication_slots tables.
> >
> > Observation:
> > Data is not lost. Replication is happening correctly. Pg_subscriber is
> > working as expected.
> >
> > Test 2:
> > - Create a 'primary' node
> > - Use normal pg_basebackup but don’t set up Physical replication
> > "./pg_basebackup –h localhost –v –W –D ../standby"
> > - Insert data before and after pg_basebackup
> > - Run pg_subscriber
> >
> > Observation:
> > Pg_subscriber command is not completing and is stuck with following
> > log repeating:
> > LOG: waiting for WAL to become available at 0/3000168
> > LOG: invalid record length at 0/3000150: expected at least 24, got 0
> >
>
> I think probably the required WAL is not copied. Can you use the -X
> option to stream WAL as well and then test? But I feel in this case
> also, we should wait for some threshold time and then exit with
> failure, removing new objects created, if any.

I have tested with -X stream option in pg_basebackup as well. In this
case also the pg_subscriber command is getting stuck.
logs:
2024-01-05 11:49:34.436 IST [61948] LOG:  invalid resource manager ID
102 at 0/3000118
2024-01-05 11:49:34.436 IST [61948] LOG:  waiting for WAL to become
available at 0/3000130

>
> > Test 3:
> > - Create a 'primary' node
> > - Use normal pg_basebackup but don’t set up Physical replication
> > "./pg_basebackup –h localhost –v –W –D ../standby"
> > -Insert data before pg_basebackup but not after pg_basebackup
> > -Run pg_subscriber
> >
> > Observation:
> > Pg_subscriber command is not completing and is stuck with following
> > log repeating:
> > LOG: waiting for WAL to become available at 0/3000168
> > LOG: invalid record length at 0/3000150: expected at least 24, got 0
> >
>
> This is similar to the previous test and you can try the same option
> here as well.
For this test as well tried with -X stream option  in pg_basebackup.
It is getting stuck here as well with similar log.

Will investigate the issue further.


Thanks and regards
Shlok Kyal

Dear Euler,

I love your proposal, so I want to join the review. Here are my first comments.

01.
Should we restrict that `--subscriber-conninfo` must not have hostname or IP?
We want users to execute pg_subscriber on the target, right?

02.
When the application was executed, many outputs filled my screen. Some of them
were by pg_subscriber, and others were server log. Can we record them into
separated file? I imagined like pg_upgrade.

03.
A replication command is used when replication slots are created. Is there a
reason to use it? I think we do not have to use logical replication walsender mode,
we can use an SQL function instead. pg_create_logical_replication_slot() also outputs
LSN, isn't it sufficient?

04.
As you know, there are several options for publications/subscriptions/replication
slots. Do you have a good way to specify them in your mind?

05.
I found that the connection string for each subscriptions have a setting
"fallback_application_name=pg_subscriber". Can we remove it?

```
postgres=# SELECT subconninfo FROM pg_subscription;
                                   subconninfo
---------------------------------------------------------------------------------
 user=postgres port=5431 fallback_application_name=pg_subscriber dbname=postgres
(1 row)
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Jan 5, 2024 at 3:36 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> I love your proposal, so I want to join the review. Here are my first comments.
>
> 01.
> Should we restrict that `--subscriber-conninfo` must not have hostname or IP?
> We want users to execute pg_subscriber on the target, right?
>

I don't see any harm in users giving those information but we should
have some checks to ensure that the server is in standby mode and is
running locally. The other related point is do we need to take input
for the target cluster directory from the user? Can't we fetch that
information once we are connected to standby?

>
> 05.
> I found that the connection string for each subscriptions have a setting
> "fallback_application_name=pg_subscriber". Can we remove it?
>
> ```
> postgres=# SELECT subconninfo FROM pg_subscription;
>                                    subconninfo
> ---------------------------------------------------------------------------------
>  user=postgres port=5431 fallback_application_name=pg_subscriber dbname=postgres
> (1 row)
> ```

Can that help distinguish the pg_subscriber connection on the publisher?


--
With Regards,
Amit Kapila.

Dear Amit,

> On Fri, Jan 5, 2024 at 3:36 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > I love your proposal, so I want to join the review. Here are my first comments.
> >
> > 01.
> > Should we restrict that `--subscriber-conninfo` must not have hostname or IP?
> > We want users to execute pg_subscriber on the target, right?
> >
> 
> I don't see any harm in users giving those information but we should
> have some checks to ensure that the server is in standby mode and is
> running locally. The other related point is do we need to take input
> for the target cluster directory from the user? Can't we fetch that
> information once we are connected to standby?

I think that functions like inet_client_addr() may be able to use, but it returns
NULL only when the connection is via a Unix-domain socket. Can we restrict
pg_subscriber to use such a socket?

> >
> > 05.
> > I found that the connection string for each subscriptions have a setting
> > "fallback_application_name=pg_subscriber". Can we remove it?
> >
> > ```
> > postgres=# SELECT subconninfo FROM pg_subscription;
> >                                    subconninfo
> >
> ---------------------------------------------------------------------------------
> >  user=postgres port=5431 fallback_application_name=pg_subscriber
> dbname=postgres
> > (1 row)
> > ```
> 
> Can that help distinguish the pg_subscriber connection on the publisher?
>

Note that this connection string is used between the publisher instance and the
subscriber instance (not pg_subscriber client application). Also, the
fallback_application_name would be replaced to the name of subscriber in
run_apply_worker()->walrcv_connect(). Actually the value would not be used.
See below output on publisher.

```
publisher=# SELECT application_name, backend_type FROM pg_stat_activity where backend_type = 'walsender';
   application_name   | backend_type 
----------------------+--------------
 pg_subscriber_5_9411 | walsender
(1 row)
```

Or, if you mean to say that this can distinguish whether the subscription is used
by pg_subscriber or not. I think it is sufficient the current format of name.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Jan 8, 2024 at 12:35 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > On Fri, Jan 5, 2024 at 3:36 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > I love your proposal, so I want to join the review. Here are my first comments.
> > >
> > > 01.
> > > Should we restrict that `--subscriber-conninfo` must not have hostname or IP?
> > > We want users to execute pg_subscriber on the target, right?
> > >
> >
> > I don't see any harm in users giving those information but we should
> > have some checks to ensure that the server is in standby mode and is
> > running locally. The other related point is do we need to take input
> > for the target cluster directory from the user? Can't we fetch that
> > information once we are connected to standby?
>
> I think that functions like inet_client_addr() may be able to use, but it returns
> NULL only when the connection is via a Unix-domain socket. Can we restrict
> pg_subscriber to use such a socket?
>

Good question. So, IIUC, this tool has a requirement to run locally
where standby is present because we want to write reconvery.conf file.
I am not sure if it is a good idea to have a restriction to use only
the unix domain socket as users need to set up the standby for that by
configuring unix_socket_directories. It is fine if we can't ensure
that it is running locally but we should at least ensure that the
server is a physical standby node to avoid the problems as Shlok has
reported.

On a related point, I see that the patch stops the standby server (if
it is running) before starting with subscriber-side steps. I was
wondering if users can object to it that there was some important data
replication in progress which this tool has stopped. Now, OTOH,
anyway, once the user uses pg_subscriber, the standby server will be
converted to a subscriber, so it may not be useful as a physical
replica. Do you or others have any thoughts on this matter?

> > >
> > > 05.
> > > I found that the connection string for each subscriptions have a setting
> > > "fallback_application_name=pg_subscriber". Can we remove it?
> > >
> > > ```
> > > postgres=# SELECT subconninfo FROM pg_subscription;
> > >                                    subconninfo
> > >
> > ---------------------------------------------------------------------------------
> > >  user=postgres port=5431 fallback_application_name=pg_subscriber
> > dbname=postgres
> > > (1 row)
> > > ```
> >
> > Can that help distinguish the pg_subscriber connection on the publisher?
> >
>
> Note that this connection string is used between the publisher instance and the
> subscriber instance (not pg_subscriber client application). Also, the
> fallback_application_name would be replaced to the name of subscriber in
> run_apply_worker()->walrcv_connect(). Actually the value would not be used.
>

Fair point. It is not clear what other purpose this can achieve,
probably Euler has something in mind for this.

--
With Regards,
Amit Kapila.

Dear Amit,

> > > I don't see any harm in users giving those information but we should
> > > have some checks to ensure that the server is in standby mode and is
> > > running locally. The other related point is do we need to take input
> > > for the target cluster directory from the user? Can't we fetch that
> > > information once we are connected to standby?
> >
> > I think that functions like inet_client_addr() may be able to use, but it returns
> > NULL only when the connection is via a Unix-domain socket. Can we restrict
> > pg_subscriber to use such a socket?
> >
> 
> Good question. So, IIUC, this tool has a requirement to run locally
> where standby is present because we want to write reconvery.conf file.
> I am not sure if it is a good idea to have a restriction to use only
> the unix domain socket as users need to set up the standby for that by
> configuring unix_socket_directories. It is fine if we can't ensure
> that it is running locally but we should at least ensure that the
> server is a physical standby node to avoid the problems as Shlok has
> reported.

While thinking more about it, I found that we did not define the policy
whether user must not connect to the target while running pg_subscriber. What
should be? If it should be avoided, some parameters like listen_addresses and
unix_socket_permissions should be restricted like start_postmaster() in
pg_upgrade/server.c. Also, the port number should be changed to another value
as well.

Personally, I vote to reject connections during the pg_subscriber.

> On a related point, I see that the patch stops the standby server (if
> it is running) before starting with subscriber-side steps. I was
> wondering if users can object to it that there was some important data
> replication in progress which this tool has stopped. Now, OTOH,
> anyway, once the user uses pg_subscriber, the standby server will be
> converted to a subscriber, so it may not be useful as a physical
> replica. Do you or others have any thoughts on this matter?

I assumed that connections should be closed before running pg_subscriber. If so,
it may be better to just fail the command when the physical standby has already
been started. There is no answer  whether data replication and user queries
should stop. Users should choose the stop option based on their policy and then
pg_subscriebr can start postmaster.
pg_upgrade does the same thing in setup().

====

Further comment:
According to the doc, currently pg_subscriber is listed in the client application.
But based on the definition, I felt it should be at "PostgreSQL Server Applications"
page. How do you think? The definition is:

>
This part contains reference information for PostgreSQL server applications and
support utilities. These commands can only be run usefully on the host where the
database server resides. Other utility programs are listed in PostgreSQL Client
Applications.
>

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Jan 9, 2024 at 12:31 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > > > I don't see any harm in users giving those information but we should
> > > > have some checks to ensure that the server is in standby mode and is
> > > > running locally. The other related point is do we need to take input
> > > > for the target cluster directory from the user? Can't we fetch that
> > > > information once we are connected to standby?
> > >
> > > I think that functions like inet_client_addr() may be able to use, but it returns
> > > NULL only when the connection is via a Unix-domain socket. Can we restrict
> > > pg_subscriber to use such a socket?
> > >
> >
> > Good question. So, IIUC, this tool has a requirement to run locally
> > where standby is present because we want to write reconvery.conf file.
> > I am not sure if it is a good idea to have a restriction to use only
> > the unix domain socket as users need to set up the standby for that by
> > configuring unix_socket_directories. It is fine if we can't ensure
> > that it is running locally but we should at least ensure that the
> > server is a physical standby node to avoid the problems as Shlok has
> > reported.
>
> While thinking more about it, I found that we did not define the policy
> whether user must not connect to the target while running pg_subscriber. What
> should be? If it should be avoided, some parameters like listen_addresses and
> unix_socket_permissions should be restricted like start_postmaster() in
> pg_upgrade/server.c.
>

Yeah, this makes sense to me.

> Also, the port number should be changed to another value
> as well.
>

Fair point, but I think in that case we should take this as one of the
parameters.

> Personally, I vote to reject connections during the pg_subscriber.
>
> > On a related point, I see that the patch stops the standby server (if
> > it is running) before starting with subscriber-side steps. I was
> > wondering if users can object to it that there was some important data
> > replication in progress which this tool has stopped. Now, OTOH,
> > anyway, once the user uses pg_subscriber, the standby server will be
> > converted to a subscriber, so it may not be useful as a physical
> > replica. Do you or others have any thoughts on this matter?
>
> I assumed that connections should be closed before running pg_subscriber. If so,
> it may be better to just fail the command when the physical standby has already
> been started. There is no answer  whether data replication and user queries
> should stop. Users should choose the stop option based on their policy and then
> pg_subscriebr can start postmaster.
> pg_upgrade does the same thing in setup().
>

Agreed.

> ====
>
> Further comment:
> According to the doc, currently pg_subscriber is listed in the client application.
> But based on the definition, I felt it should be at "PostgreSQL Server Applications"
> page. How do you think?
>

I also think it should be a server application.

--
With Regards,
Amit Kapila.

On Fri, 5 Jan 2024 at 12:19, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 4 Jan 2024 at 16:46, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Jan 4, 2024 at 12:22 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> > > Hi,
> > > I was testing the patch with following test cases:
> > >
> > > Test 1 :
> > > - Create a 'primary' node
> > > - Setup physical replica using pg_basebackup  "./pg_basebackup –h
> > > localhost –X stream –v –R –W –D ../standby "
> > > - Insert data before and after pg_basebackup
> > > - Run pg_subscriber and then insert some data to check logical
> > > replication "./pg_subscriber –D ../standby -S “host=localhost
> > > port=9000 dbname=postgres” -P “host=localhost port=9000
> > > dbname=postgres” -d postgres"
> > > - Also check pg_publication, pg_subscriber and pg_replication_slots tables.
> > >
> > > Observation:
> > > Data is not lost. Replication is happening correctly. Pg_subscriber is
> > > working as expected.
> > >
> > > Test 2:
> > > - Create a 'primary' node
> > > - Use normal pg_basebackup but don’t set up Physical replication
> > > "./pg_basebackup –h localhost –v –W –D ../standby"
> > > - Insert data before and after pg_basebackup
> > > - Run pg_subscriber
> > >
> > > Observation:
> > > Pg_subscriber command is not completing and is stuck with following
> > > log repeating:
> > > LOG: waiting for WAL to become available at 0/3000168
> > > LOG: invalid record length at 0/3000150: expected at least 24, got 0
> > >
> >
> > I think probably the required WAL is not copied. Can you use the -X
> > option to stream WAL as well and then test? But I feel in this case
> > also, we should wait for some threshold time and then exit with
> > failure, removing new objects created, if any.
>
> I have tested with -X stream option in pg_basebackup as well. In this
> case also the pg_subscriber command is getting stuck.
> logs:
> 2024-01-05 11:49:34.436 IST [61948] LOG:  invalid resource manager ID
> 102 at 0/3000118
> 2024-01-05 11:49:34.436 IST [61948] LOG:  waiting for WAL to become
> available at 0/3000130
>
> >
> > > Test 3:
> > > - Create a 'primary' node
> > > - Use normal pg_basebackup but don’t set up Physical replication
> > > "./pg_basebackup –h localhost –v –W –D ../standby"
> > > -Insert data before pg_basebackup but not after pg_basebackup
> > > -Run pg_subscriber
> > >
> > > Observation:
> > > Pg_subscriber command is not completing and is stuck with following
> > > log repeating:
> > > LOG: waiting for WAL to become available at 0/3000168
> > > LOG: invalid record length at 0/3000150: expected at least 24, got 0
> > >
> >
> > This is similar to the previous test and you can try the same option
> > here as well.
> For this test as well tried with -X stream option  in pg_basebackup.
> It is getting stuck here as well with similar log.
>
> Will investigate the issue further.

I noticed that the pg_subscriber get stuck when we run it on node
which is not a standby. It is because the of the code:
+   conn = connect_database(dbinfo[0].pubconninfo);
+   if (conn == NULL)
+       exit(1);
+   consistent_lsn = create_logical_replication_slot(conn, &dbinfo[0],
+                                                    temp_replslot);
+
.....
+else
+   {
+       appendPQExpBuffer(recoveryconfcontents, "recovery_target_lsn = '%s'\n",
+                         consistent_lsn);
+       WriteRecoveryConfig(conn, subscriber_dir, recoveryconfcontents);
+   }

Here the standby node would be waiting for the 'consistent_lsn' wal
during recovery but this wal will not be present on standby if no
physical replication is setup. Hence the command will be waiting
infinitely for the wal.
To solve this added a timeout of 60s for the recovery process and also
added a check so that pg_subscriber would give a error when it called
for node which is not in physical replication.
Have attached the patch for the same. It is a top-up patch of the
patch shared by Euler at [1].

Please review the changes and merge the changes if it looks ok.

[1] - https://www.postgresql.org/message-id/e02a2c17-22e5-4ba6-b788-de696ab74f1e%40app.fastmail.com

Thanks and regards
Shlok Kyal

On Wed, 6 Dec 2023 at 12:53, Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> > On 08.11.23 00:12, Michael Paquier wrote:
> >> - Should the subdirectory pg_basebackup be renamed into something more
> >> generic at this point?  All these things are frontend tools that deal
> >> in some way with the replication protocol to do their work.  Say
> >> a replication_tools?
> >
> > Seems like unnecessary churn.  Nobody has complained about any of the other
> > tools in there.
>
> Not sure.  We rename things across releases in the tree from time to
> time, and here that's straight-forward.
>
>
> Based on this discussion it seems we have a consensus that this tool should be
> in the pg_basebackup directory. (If/when we agree with the directory renaming,
> it could be done in a separate patch.) Besides this move, the v3 provides a dry
> run mode. It basically executes every routine but skip when should do
> modifications. It is an useful option to check if you will be able to run it
> without having issues with connectivity, permission, and existing objects
> (replication slots, publications, subscriptions). Tests were slightly improved.
> Messages were changed to *not* provide INFO messages by default and --verbose
> provides INFO messages and --verbose --verbose also provides DEBUG messages. I
> also refactored the connect_database() function into which the connection will
> always use the logical replication mode. A bug was fixed in the transient
> replication slot name. Ashutosh review [1] was included. The code was also indented.
>
> There are a few suggestions from Ashutosh [2] that I will reply in another
> email.
>
> I'm still planning to work on the following points:
>
> 1. improve the cleanup routine to point out leftover objects if there is any
>    connection issue.
> 2. remove the physical replication slot if the standby is using one
>    (primary_slot_name).
> 3. provide instructions to promote the logical replica into primary, I mean,
>    stop the replication between the nodes and remove the replication setup
>    (publications, subscriptions, replication slots). Or even include another
>    action to do it. We could add both too.
>
> Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice
> UI for users that would like to use it.

Few comments:
1) We should not allow specifying the same database name twice as we
will try to create the slots multiple times in the publisher, this can
be detected while parsing the options and error can be thrown:
+                       case 'd':
+
simple_string_list_append(&database_names, optarg);
+                               num_dbs++;
+                               break;

+static bool
+create_all_logical_replication_slots(LogicalRepInfo *dbinfo)
+{
+       int                     i;
+
+       for (i = 0; i < num_dbs; i++)
+       {
+               PGconn     *conn;
+               PGresult   *res;
+               char            replslotname[NAMEDATALEN];
....
....
....
+               /* Create replication slot on publisher. */
+               if (create_logical_replication_slot(conn, &dbinfo[i],
replslotname) != NULL || dry_run)
+                       pg_log_info("create replication slot \"%s\" on
publisher", replslotname);
+               else
+                       return false;
+
+               disconnect_database(conn);
+       }

E.g.: pg_subscriber -d postgres -d postgres

2) 2023 should be changed to 2024
+/*-------------------------------------------------------------------------
+ *
+ * pg_subscriber.c
+ *       Create a new logical replica from a standby server
+ *
+ * Copyright (C) 2023, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *             src/bin/pg_subscriber/pg_subscriber.c
+ *
+ *-------------------------------------------------------------------------
+ */

3) Similarly here too:
diff --git a/src/bin/pg_basebackup/t/040_pg_subscriber.pl
b/src/bin/pg_basebackup/t/040_pg_subscriber.pl
new file mode 100644
index 0000000000..9d20847dc2
--- /dev/null
+++ b/src/bin/pg_basebackup/t/040_pg_subscriber.pl
@@ -0,0 +1,44 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+#
+# Test checking options of pg_subscriber.
+#

4) Similarly here too:
diff --git a/src/bin/pg_basebackup/t/041_pg_subscriber_standby.pl
b/src/bin/pg_basebackup/t/041_pg_subscriber_standby.pl
new file mode 100644
index 0000000000..ce25608c68
--- /dev/null
+++ b/src/bin/pg_basebackup/t/041_pg_subscriber_standby.pl
@@ -0,0 +1,139 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+#
+# Test using a standby server as the subscriber.

Regards,
Vignesh

Here the standby node would be waiting for the 'consistent_lsn' wal

during recovery but this wal will not be present on standby if no

physical replication is setup. Hence the command will be waiting

infinitely for the wal.

To solve this added a timeout of 60s for the recovery process and also

added a check so that pg_subscriber would give a error when it called

for node which is not in physical replication.

Have attached the patch for the same. It is a top-up patch of the

patch shared by Euler at [1].

On Thu, Jan 11, 2024 at 7:59 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Wed, Jan 10, 2024, at 1:33 AM, Shlok Kyal wrote:
>
> Here the standby node would be waiting for the 'consistent_lsn' wal
> during recovery but this wal will not be present on standby if no
> physical replication is setup. Hence the command will be waiting
> infinitely for the wal.
>
>
> Hmm. Some validations are missing.
>
> To solve this added a timeout of 60s for the recovery process and also
> added a check so that pg_subscriber would give a error when it called
> for node which is not in physical replication.
> Have attached the patch for the same. It is a top-up patch of the
> patch shared by Euler at [1].
>
>
> If the user has a node that is not a standby and it does not set the GUCs to
> start the recovery process from a backup, the initial setup is broken. (That's
> the case you described.) A good UI is to detect this scenario earlier.
> Unfortunately, there isn't a reliable and cheap way to do it. You need to start
> the recovery and check if it is having some progress. (I don't have a strong
> opinion about requiring a standby to use this tool. It would reduce the
> complexity about checking if the setup has all requirements to run this tool.)
>

Right, such a check will reduce some complexity. So, +1 for the check
as proposed by Shlok. Also, what are your thoughts on a timeout during
the wait? I think it is okay to wait for 60s by default but there
should be an option for users to wait for longer.

--
With Regards,
Amit Kapila.

Dear hackers,

I have been concerned that the patch has not been tested by cfbot due to the
application error. Also, some comments were raised. Therefore, I created a patch
to move forward.
I also tried to address some comments which is not so claimed by others.
They were included in 0003 patch.

* 0001 patch
It is almost the same as v3-0001, which was posted by Euler.
An unnecessary change for Mkvcbuild.pm (this file was removed) was ignored.

* 0002 patch
This contains small fixes to keep complier quiet.

* 0003 patch
This addresses comments posted to -hackers. For now, this does not contain a doc.
Will add if everyone agrees these idea.

1.
An option --port was added to control the port number for physical standby.
Users can specify a port number via the option, or an environment variable PGSUBPORT.
If not specified, a fixed value (50111) would be used.

SOURCE: [1]

2.
A FATAL error would be raised if --subscriber-conninfo specifies non-local server.

SOURCE: [2]

3. 
Options -o/-O were added to specify options for publications/subscriptions.

SOURCE: [2]

4. 
Made standby to save their output to log file.

SOURCE: [2]

5. 
Unnecessary Assert in drop_replication_slot() was removed.

SOURCE: [3]

How do you think?
Thanks Shlok and Vignesh to work with me offline.

[1]:
https://www.postgresql.org/message-id/TY3PR01MB988978C7362A101927070D29F56A2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/TY3PR01MB9889593399165B9A04106741F5662%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/message-id/CALDaNm098Jkbh%2Bye6zMj9Ro9j1bBe6FfPV80BFbs1%3DpUuTJ07g%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

I have been concerned that the patch has not been tested by cfbot due to the

application error. Also, some comments were raised. Therefore, I created a patch

to move forward.

I have been concerned that the patch has not been tested by cfbot due to the

application error. Also, some comments were raised. Therefore, I created a patch

to move forward.

I also tried to address some comments which is not so claimed by others.

They were included in 0003 patch.

* 0001 patch

It is almost the same as v3-0001, which was posted by Euler.

An unnecessary change for Mkvcbuild.pm (this file was removed) was ignored.

* 0002 patch

This contains small fixes to keep complier quiet.

* 0003 patch

This addresses comments posted to -hackers. For now, this does not contain a doc.

Will add if everyone agrees these idea.

1.

An option --port was added to control the port number for physical standby.

Users can specify a port number via the option, or an environment variable PGSUBPORT.

If not specified, a fixed value (50111) would be used.

2.

A FATAL error would be raised if --subscriber-conninfo specifies non-local server.

3. 

Options -o/-O were added to specify options for publications/subscriptions.

4. 

Made standby to save their output to log file.

5. 

Unnecessary Assert in drop_replication_slot() was removed.

On Fri, Jan 12, 2024 at 4:30 AM Euler Taveira <euler@eulerto.com> wrote:
>
>
> 3.
> Options -o/-O were added to specify options for publications/subscriptions.
>
>
> Flexibility is cool. However, I think the cost benefit of it is not good. You
> have to parse the options to catch preliminary errors. Things like publish only
> delete and subscription options that conflicts with the embedded ones are
> additional sources of failure.
>

Yeah, I am also not sure we need those. Did we discussed about that
previously? OTOH, we may consider to enhance this tool later if we
have user demand for such options.

BTW, I think we need some way to at least drop the existing
subscriptions otherwise the newly created subscriber will attempt to
fetch the data which may not be intended. Ashutosh made an argument
above thread that we need an option for publications as well.

--
With Regards,
Amit Kapila.

Dear Amit, Euler,

> >
> >
> > 3.
> > Options -o/-O were added to specify options for publications/subscriptions.
> >
> >
> > Flexibility is cool. However, I think the cost benefit of it is not good. You
> > have to parse the options to catch preliminary errors. Things like publish only
> > delete and subscription options that conflicts with the embedded ones are
> > additional sources of failure.
> >
> 
> Yeah, I am also not sure we need those. Did we discussed about that
> previously? OTOH, we may consider to enhance this tool later if we
> have user demand for such options.

OK, so let's drop it once and consider later as an enhancement.
As Euler said, it leads additional ERRORs.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Euler,

Sorry for disturbing your work and thanks for updates.
I will review your patch again.

>
* 0001 patch
It is almost the same as v3-0001, which was posted by Euler.
An unnecessary change for Mkvcbuild.pm (this file was removed) was ignored.

v5 removes the MSVC support.
>

Confirmed that the patch could be applied.

>
* 0002 patch
This contains small fixes to keep complier quiet.

I applied it. Although, I used a different approach for format specifier.
>

Good, all warnings were removed. However, the patch failed to pass tests on FreeBSD twice.
 I'm quite not sure the ERROR, but is it related with us?

>
* 0003 patch
This addresses comments posted to -hackers. For now, this does not contain a doc.
Will add if everyone agrees these idea.

I didn't review all items but ...

1.
An option --port was added to control the port number for physical standby.
Users can specify a port number via the option, or an environment variable PGSUBPORT.
If not specified, a fixed value (50111) would be used.

My first reaction as a new user would be: why do I need to specify a port if my
--subscriber-conninfo already contains a port? Ugh. I'm wondering if we can do
it behind the scenes. Try a range of ports.
>

My initial motivation of the setting was to avoid establishing connections
during the pg_subscriber. While considering more, I started to think that
--subscriber-conninfo may not be needed. pg_upgrade does not requires the
string: it requries username, and optionally port number (which would be used
during the upgrade) instead. The advantage of this approach is that we do not
have to parse the connection string.
How do you think?

>
2.
A FATAL error would be raised if --subscriber-conninfo specifies non-local server.

Extra protection is always good. However, let's make sure this code path is
really useful. I'll think a bit about it.
>

OK, I can wait your consideration. Note that if we follow the pg_ugprade style,
we may able to reuse check_pghost_envvar().

>
3.
Options -o/-O were added to specify options for publications/subscriptions.

Flexibility is cool. However, I think the cost benefit of it is not good. You
have to parse the options to catch preliminary errors. Things like publish only
delete and subscription options that conflicts with the embedded ones are
additional sources of failure.
>

As I already replied, let's stop doing it once. We can resume based on the requirement.

>
4.
Made standby to save their output to log file.

It was already done in v5. I did in a different way.
>

Good. I felt that yours were better. BTW, can we record outputs by pg_subscriber to a file as well?
pg_upgrade did similar thing. Thought?

>
5.
Unnecessary Assert in drop_replication_slot() was removed.

Instead, I fixed the code and keep the assert.
>

Cool.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Euler,

Here are comments for your v5 patch.

01.
In the document, two words target/standby are used as almost the same meaning.
Can you unify them?

02.
```
 <refsynopsisdiv>
  <cmdsynopsis>
   <command>pg_subscriber</command>
   <arg rep="repeat"><replaceable>option</replaceable></arg>
  </cmdsynopsis>
 </refsynopsisdiv>
```

There are some mandatory options like -D/-S/-P. It must be listed in Synopsis chapter.

03.
```
  <para>
   <application>pg_subscriber</application> takes the publisher and subscriber
   connection strings, a cluster directory from a standby server and a list of
   database names and it sets up a new logical replica using the physical
   recovery process.
  </para>
```

I briefly checked other pages and they do not describe accepted options here.
A summary of the application should be mentioned. Based on that, how about:

```
pg_subscriber creates a new <link linkend="logical-replication-subscription">
subscriber</link> from a physical standby server. This allows users to quickly
set up logical replication system.
```

04.
```
  <para>
   The <application>pg_subscriber</application> should be run at the target
   server. The source server (known as publisher server) should accept logical
   replication connections from the target server (known as subscriber server).
   The target server should accept local logical replication connection.
  </para>
```

I'm not native speaker, but they are not just recommmendations - they are surely
required. So, should we replace s/should/has to/?

05.
```
     <varlistentry>
      <term><option>-S <replaceable class="parameter">conninfo</replaceable></option></term>
      <term><option>--subscriber-conninfo=<replaceable class="parameter">conninfo</replaceable></option></term>
      <listitem>
       <para>
        The connection string to the subscriber. For details see <xref linkend="libpq-connstring"/>.
       </para>
      </listitem>
     </varlistentry>
```

I became not sure whether it is "The connection string to the subscriber.".
The server is still physical standby at that time.

06.
```
 * IDENTIFICATION
 *        src/bin/pg_subscriber/pg_subscriber.c
```

The identification is not correct.

07.
I felt that there were too many global variables and LogicalRepInfo should be
refactored. Because...

* Some info related with clusters(e.g., subscriber_dir, conninfo, ...) should be
  gathered in one struct.
* pubconninfo/subsconninfo are stored per db, but it is not needed if we have
  one base_conninfo.
* pubname/subname are not needed because we have fixed naming rule.
* pg_ctl_path and pg_resetwal_path can be conbimed into one bindir.
* num_dbs should not be alone.
...

Based on above, how about using structures like below?

```
typedef struct LogicalRepPerdbInfo
{
    Oid        oid;
    char   *dbname;
    bool    made_replslot;    /* replication slot was created */
    bool    made_publication;    /* publication was created */
    bool    made_subscription;    /* subscription was created */
} LogicalRepPerdbInfo;

typedef struct PrimaryInfo
{
    char   *base_conninfo;
    bool    made_transient_replslot;
} PrimaryInfo;

typedef struct StandbyInfo
{
    char   *base_conninfo;
    char   *bindir;
    char   *pgdata;
    char   *primary_slot_name;
} StandbyInfo;

typedef struct LogicalRepInfo
{
    int                        num_dbs;
    LogicalRepPerdbInfo       *perdb;
    PrimaryInfo               *primary;
    StandbyInfo               *standby;
} LogicalRepInfo;
```

08.
```
    char       *subconninfo;    /* subscription connection string for logical
                                 * replication */
```

Not sure how we should notate because the target has not been subscriber yet.

09.
```
enum WaitPMResult
{
    POSTMASTER_READY,
    POSTMASTER_STANDBY,
    POSTMASTER_STILL_STARTING,
    POSTMASTER_FAILED
};
```

This enum has been already defined in pg_ctl.c. Not sure we can use the same name.
Can we rename to PGSWaitPMResult. or export pre-existing one?

10.
```
/* Options */
static const char *progname;
```

I think it is not an option.

11.
```
/*
 * Validate a connection string. Returns a base connection string that is a
 * connection string without a database name plus a fallback application name.
 * Since we might process multiple databases, each database name will be
 * appended to this base connection string to provide a final connection string.
 * If the second argument (dbname) is not null, returns dbname if the provided
 * connection string contains it. If option --database is not provided, uses
 * dbname as the only database to setup the logical replica.
 * It is the caller's responsibility to free the returned connection string and
 * dbname.
 */
static char *
get_base_conninfo(char *conninfo, char *dbname, const char *noderole)
```
Just FYI - adding fallback_application_name may be too optimisitic. Currently
the output was used by both pg_subscriber and subscription connection.

12.
Can we add an option not to remove log files even operations were succeeded.

13.
```
        /*
         * Since the standby server is running, check if it is using an
         * existing replication slot for WAL retention purposes. This
         * replication slot has no use after the transformation, hence, it
         * will be removed at the end of this process.
         */
        primary_slot_name = use_primary_slot_name();
```

Now primary_slot_name is checked only when the server have been started, but
it should be checked in any cases.

14.
```
    consistent_lsn = create_logical_replication_slot(conn, &dbinfo[0],
                                                     temp_replslot);
```

Can we create a temporary slot here?

15.
I found that subscriptions cannot be started if tuples are inserted on publisher
after creating temp_replslot. After starting a subscriber, I got below output on the log.

```
ERROR:  could not receive data from WAL stream: ERROR:  publication "pg_subscriber_5" does not exist
CONTEXT:  slot "pg_subscriber_5_3632", output plugin "pgoutput", in the change callback, associated LSN 0/30008A8
LOG:  background worker "logical replication apply worker" (PID 3669) exited with exit code 1
```

But this is strange. I confirmed that the specified publication surely exists.
Do you know the reason?

```
publisher=# SELECT pubname FROM pg_publication;
     pubname
-----------------
 pg_subscriber_5
(1 row)
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

On Fri, 12 Jan 2024 at 09:32, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Good, all warnings were removed. However, the patch failed to pass tests on FreeBSD twice.
>  I'm quite not sure the ERROR, but is it related with us?

FreeBSD errors started after FreeBSD's CI image was updated [1]. I do
not think error is related to this.

[1] https://cirrus-ci.com/task/4700394639589376

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

On Fri, Jan 12, 2024 at 12:02 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> I didn't review all items but ...
>
> 1.
> An option --port was added to control the port number for physical standby.
> Users can specify a port number via the option, or an environment variable PGSUBPORT.
> If not specified, a fixed value (50111) would be used.
>
> My first reaction as a new user would be: why do I need to specify a port if my
> --subscriber-conninfo already contains a port? Ugh. I'm wondering if we can do
> it behind the scenes. Try a range of ports.
> >
>
> My initial motivation of the setting was to avoid establishing connections
> during the pg_subscriber. While considering more, I started to think that
> --subscriber-conninfo may not be needed. pg_upgrade does not requires the
> string: it requries username, and optionally port number (which would be used
> during the upgrade) instead. The advantage of this approach is that we do not
> have to parse the connection string.
> How do you think?
>

+1. This seems worth considering. I think unless we have a good reason
to have this parameter, we should try to avoid it.

--
With Regards,
Amit Kapila.

Hi Euler,

On Fri, Jan 12, 2024 at 6:16 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Jan 11, 2024, at 9:18 AM, Hayato Kuroda (Fujitsu) wrote:
>
> I have been concerned that the patch has not been tested by cfbot due to the
> application error. Also, some comments were raised. Therefore, I created a patch
> to move forward.
>
>
> Let me send an updated patch to hopefully keep the CF bot happy. The following
> items are included in this patch:
>
> * drop physical replication slot if standby is using one [1].
> * cleanup small changes (copyright, .gitignore) [2][3]
> * fix getopt_long() options [2]
> * fix format specifier for some messages
> * move doc to Server Application section [4]
> * fix assert failure
> * ignore duplicate database names [2]
> * store subscriber server log into a separate file
> * remove MSVC support
>
> I'm still addressing other reviews and I'll post another version that includes
> it soon.
>
> [1] https://www.postgresql.org/message-id/e02a2c17-22e5-4ba6-b788-de696ab74f1e%40app.fastmail.com
> [2] https://www.postgresql.org/message-id/CALDaNm1joke42n68LdegN5wCpaeoOMex2EHcdZrVZnGD3UhfNQ%40mail.gmail.com
> [3]
https://www.postgresql.org/message-id/TY3PR01MB98895BA6C1D72CB8582CACC4F5682%40TY3PR01MB9889.jpnprd01.prod.outlook.com
> [4]
https://www.postgresql.org/message-id/TY3PR01MB988978C7362A101927070D29F56A2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
>

+ <refnamediv>
+  <refname>pg_subscriber</refname>
+  <refpurpose>create a new logical replica from a standby server</refpurpose>
+ </refnamediv>
I'm a bit confused about this wording because we are converting a standby
to a logical replica but not creating a new logical replica and leaving the
standby as is. How about:

<refpurpose>convert a standby replica to a logical replica</refpurpose>

+  <para>
+   The <application>pg_subscriber</application> should be run at the target
+   server. The source server (known as publisher server) should accept logical
+   replication connections from the target server (known as subscriber server).
+   The target server should accept local logical replication connection.
+  </para>

What is *local logical replication*? I can't find any clue in the patch, can you
give me some hint?


>
> --
> Euler Taveira
> EDB   https://www.enterprisedb.com/
>


--
Regards
Junwang Zhao

On Thu, Dec 21, 2023 at 11:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Dec 6, 2023 at 12:53 PM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote:
> >
> > On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> > > On 08.11.23 00:12, Michael Paquier wrote:
> > >> - Should the subdirectory pg_basebackup be renamed into something more
> > >> generic at this point?  All these things are frontend tools that deal
> > >> in some way with the replication protocol to do their work.  Say
> > >> a replication_tools?
> > >
> > > Seems like unnecessary churn.  Nobody has complained about any of the other
> > > tools in there.
> >
> > Not sure.  We rename things across releases in the tree from time to
> > time, and here that's straight-forward.
> >
> >
> > Based on this discussion it seems we have a consensus that this tool should be
> > in the pg_basebackup directory. (If/when we agree with the directory renaming,
> > it could be done in a separate patch.) Besides this move, the v3 provides a dry
> > run mode. It basically executes every routine but skip when should do
> > modifications. It is an useful option to check if you will be able to run it
> > without having issues with connectivity, permission, and existing objects
> > (replication slots, publications, subscriptions). Tests were slightly improved.
> > Messages were changed to *not* provide INFO messages by default and --verbose
> > provides INFO messages and --verbose --verbose also provides DEBUG messages. I
> > also refactored the connect_database() function into which the connection will
> > always use the logical replication mode. A bug was fixed in the transient
> > replication slot name. Ashutosh review [1] was included. The code was also indented.
> >
> > There are a few suggestions from Ashutosh [2] that I will reply in another
> > email.
> >
> > I'm still planning to work on the following points:
> >
> > 1. improve the cleanup routine to point out leftover objects if there is any
> >    connection issue.
> >
>
> I think this is an important part. Shall we try to write to some file
> the pending objects to be cleaned up? We do something like that during
> the upgrade.
>
> > 2. remove the physical replication slot if the standby is using one
> >    (primary_slot_name).
> > 3. provide instructions to promote the logical replica into primary, I mean,
> >    stop the replication between the nodes and remove the replication setup
> >    (publications, subscriptions, replication slots). Or even include another
> >    action to do it. We could add both too.
> >
> > Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice
> > UI for users that would like to use it.
> >
>
> Isn't point 2 also essential because how would otherwise such a slot
> be advanced or removed?
>
> A few other points:
> ==============
> 1. Previously, I asked whether we need an additional replication slot
> patch created to get consistent LSN and I see the following comment in
> the patch:
>
> + *
> + * XXX we should probably use the last created replication slot to get a
> + * consistent LSN but it should be changed after adding pg_basebackup
> + * support.
>
> Yeah, sure, we may want to do that after backup support and we can
> keep a comment for the same but I feel as the patch stands today,
> there is no good reason to keep it. Also, is there a reason that we
> can't create the slots after backup is complete and before we write
> recovery parameters
>
> 2.
> + appendPQExpBuffer(str,
> +   "CREATE SUBSCRIPTION %s CONNECTION '%s' PUBLICATION %s "
> +   "WITH (create_slot = false, copy_data = false, enabled = false)",
> +   dbinfo->subname, dbinfo->pubconninfo, dbinfo->pubname);
>
> Shouldn't we enable two_phase by default for newly created
> subscriptions? Is there a reason for not doing so?
>
> 3. How about sync slots on the physical standby if present? Do we want
> to retain those as it is or do we need to remove those? We are
> actively working on the patch [1] for the same.
>
> 4. Can we see some numbers with various sizes of databases (cluster)
> to see how it impacts the time for small to large-size databases as
> compared to the traditional method? This might help us with giving
> users advice on when to use this tool. We can do this bit later as
> well when the patch is closer to being ready for commit.

I have done the Performance testing and attached the results to
compare the 'Execution Time' between 'logical replication' and
'pg_subscriber' for 100MB, 1GB and 5GB data:
                            | 100MB | 1GB      | 5GB
Logical rep (2 w) | 1.815s  | 14.895s | 75.541s
Logical rep (4 w) | 1.194s  | 9.484s   | 46.938s
Logical rep (8 w) | 0.828s  | 6.422s   | 31.704s
Logical rep(10 w)| 0.646s  | 3.843s   | 18.425s
pg_subscriber     | 3.977s  | 9.988s   | 12.665s

Here, 'w' stands for 'workers'. I have included the tests to see the
test result variations with different values for
'max_sync_workers_per_subscription' ranging from 2 to 10. I ran the
tests for different data records; for 100MB I put  3,00,000 Records,
for 1GB I put 30,00,000 Records and for 5GB I put 1,50,00,000 Records.
It is observed that 'pg_subscriber' is better when the table size is
more.
Next I plan to run these tests for 10GB and 20GB to see if this trend
continues or not.
Attaching the script files which have the details of the test scripts
used and the excel file has the test run details. The
'pg_subscriber.pl' file is to test with 'pg_subscriber' and the
'logical_rep.pl' file is to test with 'Logical Replication'.

Thanks and Regards,
Shubham Khanna.

Dear Euler, hackers,

I found that some bugs which have been reported by Shlok were not fixed, so
made a top-up patch. 0001 was not changed, and 0002 contains below:

* Add a timeout option for the recovery option, per [1]. The code was basically ported from pg_ctl.c.
* Reject if the target server is not a standby, per [2]
* Raise FATAL error if --subscriber-conninfo specifies non-local server, per [3]
  (not sure it is really needed, so feel free reject the part.)

Feel free to merge parts of 0002 if it looks good to you.
Thanks Shlok to make a part of patch.

[1]: https://www.postgresql.org/message-id/CANhcyEUCt-g4JLQU3Q3ofFk_Vt-Tqh3ZdXoLcpT8fjz9LY_-ww%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CANhcyEUCt-g4JLQU3Q3ofFk_Vt-Tqh3ZdXoLcpT8fjz9LY_-ww%40mail.gmail.com
[3]:
https://www.postgresql.org/message-id/TY3PR01MB98895BA6C1D72CB8582CACC4F5682%40TY3PR01MB9889.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

I have some comments for the v5 patch:

1.
```
+ base_dir = (char *) pg_malloc0(MAXPGPATH);
+ len = snprintf(base_dir, MAXPGPATH, "%s/%s", subscriber_dir, PGS_OUTPUT_DIR);
```
Before these lines, I think we should use
'canonicalize_path(subscriber_dir)' to remove extra unnecessary
characters. This function is used in many places like initdb.c,
pg_ctl.c, pg_basebakup.c, etc

2.
I also feels that there are many global variables and can be arranged
as structures as suggested by Kuroda-san in [1]

[1]
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com

Thanks and Regards
Shlok Kyal


On Fri, 12 Jan 2024 at 03:46, Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Jan 11, 2024, at 9:18 AM, Hayato Kuroda (Fujitsu) wrote:
>
> I have been concerned that the patch has not been tested by cfbot due to the
> application error. Also, some comments were raised. Therefore, I created a patch
> to move forward.
>
>
> Let me send an updated patch to hopefully keep the CF bot happy. The following
> items are included in this patch:
>
> * drop physical replication slot if standby is using one [1].
> * cleanup small changes (copyright, .gitignore) [2][3]
> * fix getopt_long() options [2]
> * fix format specifier for some messages
> * move doc to Server Application section [4]
> * fix assert failure
> * ignore duplicate database names [2]
> * store subscriber server log into a separate file
> * remove MSVC support
>
> I'm still addressing other reviews and I'll post another version that includes
> it soon.
>
> [1] https://www.postgresql.org/message-id/e02a2c17-22e5-4ba6-b788-de696ab74f1e%40app.fastmail.com
> [2] https://www.postgresql.org/message-id/CALDaNm1joke42n68LdegN5wCpaeoOMex2EHcdZrVZnGD3UhfNQ%40mail.gmail.com
> [3]
https://www.postgresql.org/message-id/TY3PR01MB98895BA6C1D72CB8582CACC4F5682%40TY3PR01MB9889.jpnprd01.prod.outlook.com
> [4]
https://www.postgresql.org/message-id/TY3PR01MB988978C7362A101927070D29F56A2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
>
>
> --
> Euler Taveira
> EDB   https://www.enterprisedb.com/
>

On 11.01.24 23:15, Euler Taveira wrote:
> A new tool called pg_subscriber can convert a physical replica into a 
> logical replica. It runs on the target server and should be able to 
> connect to the source server (publisher) and the target server (subscriber).

Can we have a discussion on the name?

I find the name pg_subscriber too general.

The replication/backup/recovery tools in PostgreSQL are usually named 
along the lines of "verb - object".  (Otherwise, they would all be 
called "pg_backup"??)  Moreover, "pg_subscriber" also sounds like the 
name of the program that runs the subscriber itself, like what the 
walreceiver does now.

Very early in this thread, someone mentioned the name 
pg_create_subscriber, and of course there is pglogical_create_subscriber 
as the historical predecessor.  Something along those lines seems better 
to me.  Maybe there are other ideas.

On Thu, Jan 18, 2024 at 2:49 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 11.01.24 23:15, Euler Taveira wrote:
> > A new tool called pg_subscriber can convert a physical replica into a
> > logical replica. It runs on the target server and should be able to
> > connect to the source server (publisher) and the target server (subscriber).
>
> Can we have a discussion on the name?
>
> I find the name pg_subscriber too general.
>
> The replication/backup/recovery tools in PostgreSQL are usually named
> along the lines of "verb - object".  (Otherwise, they would all be
> called "pg_backup"??)  Moreover, "pg_subscriber" also sounds like the
> name of the program that runs the subscriber itself, like what the
> walreceiver does now.
>
> Very early in this thread, someone mentioned the name
> pg_create_subscriber, and of course there is pglogical_create_subscriber
> as the historical predecessor.  Something along those lines seems better
> to me.  Maybe there are other ideas.
>

The other option could be pg_createsubscriber on the lines of
pg_verifybackup and pg_combinebackup. Yet other options could be
pg_buildsubscriber, pg_makesubscriber as 'build' or 'make' in the name
sounds like we are doing some work to create the subscriber which I
think is the case here.

--
With Regards,
Amit Kapila.

On Tue, Jan 16, 2024 at 11:58 AM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Thu, Dec 21, 2023 at 11:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > 4. Can we see some numbers with various sizes of databases (cluster)
> > to see how it impacts the time for small to large-size databases as
> > compared to the traditional method? This might help us with giving
> > users advice on when to use this tool. We can do this bit later as
> > well when the patch is closer to being ready for commit.
>
> I have done the Performance testing and attached the results to
> compare the 'Execution Time' between 'logical replication' and
> 'pg_subscriber' for 100MB, 1GB and 5GB data:
>                             | 100MB | 1GB      | 5GB
> Logical rep (2 w) | 1.815s  | 14.895s | 75.541s
> Logical rep (4 w) | 1.194s  | 9.484s   | 46.938s
> Logical rep (8 w) | 0.828s  | 6.422s   | 31.704s
> Logical rep(10 w)| 0.646s  | 3.843s   | 18.425s
> pg_subscriber     | 3.977s  | 9.988s   | 12.665s
>
> Here, 'w' stands for 'workers'. I have included the tests to see the
> test result variations with different values for
> 'max_sync_workers_per_subscription' ranging from 2 to 10. I ran the
> tests for different data records; for 100MB I put  3,00,000 Records,
> for 1GB I put 30,00,000 Records and for 5GB I put 1,50,00,000 Records.
> It is observed that 'pg_subscriber' is better when the table size is
> more.
>

Thanks for the tests. IIUC, it shows for smaller data this tool can
take more time. Can we do perf to see if there is something we can do
about reducing the overhead?

> Next I plan to run these tests for 10GB and 20GB to see if this trend
> continues or not.
>

Okay, that makes sense.

With Regards,
Amit Kapila.

On Tue, Jan 16, 2024 at 11:58 AM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Thu, Dec 21, 2023 at 11:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Dec 6, 2023 at 12:53 PM Euler Taveira <euler@eulerto.com> wrote:
> > >
> > > On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote:
> > >
> > > On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote:
> > > > On 08.11.23 00:12, Michael Paquier wrote:
> > > >> - Should the subdirectory pg_basebackup be renamed into something more
> > > >> generic at this point?  All these things are frontend tools that deal
> > > >> in some way with the replication protocol to do their work.  Say
> > > >> a replication_tools?
> > > >
> > > > Seems like unnecessary churn.  Nobody has complained about any of the other
> > > > tools in there.
> > >
> > > Not sure.  We rename things across releases in the tree from time to
> > > time, and here that's straight-forward.
> > >
> > >
> > > Based on this discussion it seems we have a consensus that this tool should be
> > > in the pg_basebackup directory. (If/when we agree with the directory renaming,
> > > it could be done in a separate patch.) Besides this move, the v3 provides a dry
> > > run mode. It basically executes every routine but skip when should do
> > > modifications. It is an useful option to check if you will be able to run it
> > > without having issues with connectivity, permission, and existing objects
> > > (replication slots, publications, subscriptions). Tests were slightly improved.
> > > Messages were changed to *not* provide INFO messages by default and --verbose
> > > provides INFO messages and --verbose --verbose also provides DEBUG messages. I
> > > also refactored the connect_database() function into which the connection will
> > > always use the logical replication mode. A bug was fixed in the transient
> > > replication slot name. Ashutosh review [1] was included. The code was also indented.
> > >
> > > There are a few suggestions from Ashutosh [2] that I will reply in another
> > > email.
> > >
> > > I'm still planning to work on the following points:
> > >
> > > 1. improve the cleanup routine to point out leftover objects if there is any
> > >    connection issue.
> > >
> >
> > I think this is an important part. Shall we try to write to some file
> > the pending objects to be cleaned up? We do something like that during
> > the upgrade.
> >
> > > 2. remove the physical replication slot if the standby is using one
> > >    (primary_slot_name).
> > > 3. provide instructions to promote the logical replica into primary, I mean,
> > >    stop the replication between the nodes and remove the replication setup
> > >    (publications, subscriptions, replication slots). Or even include another
> > >    action to do it. We could add both too.
> > >
> > > Point 1 should be done. Points 2 and 3 aren't essential but will provide a nice
> > > UI for users that would like to use it.
> > >
> >
> > Isn't point 2 also essential because how would otherwise such a slot
> > be advanced or removed?
> >
> > A few other points:
> > ==============
> > 1. Previously, I asked whether we need an additional replication slot
> > patch created to get consistent LSN and I see the following comment in
> > the patch:
> >
> > + *
> > + * XXX we should probably use the last created replication slot to get a
> > + * consistent LSN but it should be changed after adding pg_basebackup
> > + * support.
> >
> > Yeah, sure, we may want to do that after backup support and we can
> > keep a comment for the same but I feel as the patch stands today,
> > there is no good reason to keep it. Also, is there a reason that we
> > can't create the slots after backup is complete and before we write
> > recovery parameters
> >
> > 2.
> > + appendPQExpBuffer(str,
> > +   "CREATE SUBSCRIPTION %s CONNECTION '%s' PUBLICATION %s "
> > +   "WITH (create_slot = false, copy_data = false, enabled = false)",
> > +   dbinfo->subname, dbinfo->pubconninfo, dbinfo->pubname);
> >
> > Shouldn't we enable two_phase by default for newly created
> > subscriptions? Is there a reason for not doing so?
> >
> > 3. How about sync slots on the physical standby if present? Do we want
> > to retain those as it is or do we need to remove those? We are
> > actively working on the patch [1] for the same.
> >
> > 4. Can we see some numbers with various sizes of databases (cluster)
> > to see how it impacts the time for small to large-size databases as
> > compared to the traditional method? This might help us with giving
> > users advice on when to use this tool. We can do this bit later as
> > well when the patch is closer to being ready for commit.
>
> I have done the Performance testing and attached the results to
> compare the 'Execution Time' between 'logical replication' and
> 'pg_subscriber' for 100MB, 1GB and 5GB data:
>                             | 100MB | 1GB      | 5GB
> Logical rep (2 w) | 1.815s  | 14.895s | 75.541s
> Logical rep (4 w) | 1.194s  | 9.484s   | 46.938s
> Logical rep (8 w) | 0.828s  | 6.422s   | 31.704s
> Logical rep(10 w)| 0.646s  | 3.843s   | 18.425s
> pg_subscriber     | 3.977s  | 9.988s   | 12.665s
>
> Here, 'w' stands for 'workers'. I have included the tests to see the
> test result variations with different values for
> 'max_sync_workers_per_subscription' ranging from 2 to 10. I ran the
> tests for different data records; for 100MB I put  3,00,000 Records,
> for 1GB I put 30,00,000 Records and for 5GB I put 1,50,00,000 Records.
> It is observed that 'pg_subscriber' is better when the table size is
> more.
> Next I plan to run these tests for 10GB and 20GB to see if this trend
> continues or not.

I have done the Performance testing and attached the results to
compare the 'Execution Time' between 'logical replication' and
'pg_subscriber' for 10GB and 20GB data:
                            | 10GB     | 20GB
Logical rep (2 w) | 157.131s| 343.191s
Logical rep (4 w) | 116.627s| 240.480s
Logical rep (8 w) | 95.237s  | 275.715s
Logical rep(10 w)| 92.792s  | 280.538s
pg_subscriber     | 22.734s  | 25.661s

As expected, we can see that pg_subscriber is very much better in
ideal cases with approximately 7x times better in case of 10GB and 13x
times better in case of 20GB.
I'm attaching the script files which have the details of the test
scripts used and the excel file has the test run details. The
'pg_subscriber.pl' file is for 'Streaming Replication' and the
'logical_replication.pl' file is for 'Logical Replication'.
Note: For 20GB the record count should be changed to 6,00,00,000 and
the 'max_sync_workers_per_subscription' needs to be adjusted for
different logical replication tests with different workers.

Thanks and Regards,
Shubham Khanna.

On 18.01.24 10:37, Amit Kapila wrote:
> The other option could be pg_createsubscriber on the lines of
> pg_verifybackup and pg_combinebackup.

Yes, that spelling would be more consistent.

> Yet other options could be
> pg_buildsubscriber, pg_makesubscriber as 'build' or 'make' in the name
> sounds like we are doing some work to create the subscriber which I
> think is the case here.

I see your point here.  pg_createsubscriber is not like createuser in 
that it just runs an SQL command.  It does something different than 
CREATE SUBSCRIBER.  So a different verb would make that clearer.  Maybe 
something from here: https://www.thesaurus.com/browse/convert

Dear hackers,

>
> 15.
> I found that subscriptions cannot be started if tuples are inserted on publisher
> after creating temp_replslot. After starting a subscriber, I got below output on the
> log.
>
> ```
> ERROR:  could not receive data from WAL stream: ERROR:  publication
> "pg_subscriber_5" does not exist
> CONTEXT:  slot "pg_subscriber_5_3632", output plugin "pgoutput", in the change
> callback, associated LSN 0/30008A8
> LOG:  background worker "logical replication apply worker" (PID 3669) exited
> with exit code 1
> ```
>
> But this is strange. I confirmed that the specified publication surely exists.
> Do you know the reason?
>
> ```
> publisher=# SELECT pubname FROM pg_publication;
>      pubname
> -----------------
>  pg_subscriber_5
> (1 row)
> ```
>

I analyzed and found a reason. This is because publications are invisible for some transactions.

As the first place, below operations were executed in this case.
Tuples were inserted after getting consistent_lsn, but before starting the standby.
After doing the workload, I confirmed again that the publication was created.

1. on primary, logical replication slots were created.
2. on primary, another replication slot was created.
3. ===on primary, some tuples were inserted. ===
4. on standby, a server process was started
5. on standby, the process waited until all changes have come.
6. on primary, publications were created.
7. on standby, subscriptions were created.
8. on standby, a replication progress for each subscriptions was set to given LSN (got at step2).
=====pg_subscriber finished here=====
9. on standby, a server process was started again
10. on standby, subscriptions were enabled. They referred slots created at step1.
11. on primary, decoding was started but ERROR was raised.

In this case, tuples were inserted *before creating publication*.
So I thought that the decoded transaction could not see the publication because
it was committed after insertions.

One solution is to create a publication before creating a consistent slot.
Changes which came before creating the slot were surely replicated to the standby,
so upcoming transactions can see the object. We are planning to patch set to fix
the issue in this approach.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Peter,

> 
> > Yet other options could be
> > pg_buildsubscriber, pg_makesubscriber as 'build' or 'make' in the name
> > sounds like we are doing some work to create the subscriber which I
> > think is the case here.
> 
> I see your point here.  pg_createsubscriber is not like createuser in
> that it just runs an SQL command.  It does something different than
> CREATE SUBSCRIBER.  So a different verb would make that clearer.  Maybe
> something from here: https://www.thesaurus.com/browse/convert

I read the link and found a good verb "switch". So, how about using "pg_switchsubscriber"?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Jan 22, 2024 at 2:38 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
> >
> > > Yet other options could be
> > > pg_buildsubscriber, pg_makesubscriber as 'build' or 'make' in the name
> > > sounds like we are doing some work to create the subscriber which I
> > > think is the case here.
> >
> > I see your point here.  pg_createsubscriber is not like createuser in
> > that it just runs an SQL command.  It does something different than
> > CREATE SUBSCRIBER.

Right.

  So a different verb would make that clearer.  Maybe
> > something from here: https://www.thesaurus.com/browse/convert
>
> I read the link and found a good verb "switch". So, how about using "pg_switchsubscriber"?
>

I also initially thought on these lines and came up with a name like
pg_convertsubscriber but didn't feel strongly about it as that would
have sounded meaningful if we use a name like
pg_convertstandbytosubscriber. Now, that has become too long. Having
said that, I am not opposed to it having a name on those lines. BTW,
another option that occurred to me today is pg_preparesubscriber. We
internally create slots and then wait for wal, etc. which makes me
sound like adding 'prepare' in the name can also explain the purpose.

--
With Regards,
Amit Kapila.

Dear Euler, hackers,

We fixed some of the comments posted in the thread. We have created it
as top-up patch 0002 and 0003.

0002 patch contains the following changes:
* Add a timeout option for the recovery option, per [1]. The code was
basically ported from pg_ctl.c.
* Reject if the target server is not a standby, per [2]
* Raise FATAL error if --subscriber-conninfo specifies non-local server, per [3]
  (not sure it is really needed, so feel free reject the part.)
* Add check for max_replication_slots and wal_level; as per [4]
* Add -u and -p options; as per [5]
* Addressed comment except 5 and 8 in [6] and comment in [7]

0003 patch contains fix for bug reported in [8].

Feel free to merge parts of 0002 and 0003 if it looks good to you.
Thanks Kuroda-san to make patch 0003 and a part of patch 0002.

[1]: https://www.postgresql.org/message-id/CANhcyEUCt-g4JLQU3Q3ofFk_Vt-Tqh3ZdXoLcpT8fjz9LY_-ww%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CANhcyEUCt-g4JLQU3Q3ofFk_Vt-Tqh3ZdXoLcpT8fjz9LY_-ww%40mail.gmail.com
[3]:
https://www.postgresql.org/message-id/TY3PR01MB98895BA6C1D72CB8582CACC4F5682%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[4]: https://www.postgresql.org/message-id/CALDaNm098Jkbh%2Bye6zMj9Ro9j1bBe6FfPV80BFbs1%3DpUuTJ07g%40mail.gmail.com
[5]: https://www.postgresql.org/message-id/CAA4eK1JB_ko7a5JMS3WfAn583RadAKCDhiE9JgmfMA8ZZ5xcQw%40mail.gmail.com
[6]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[7]: https://www.postgresql.org/message-id/CANhcyEXjGmryoZPACS_i-joqvcz5e6Zb3u4g38SAy_iSTGhShg%40mail.gmail.com
[8]:
https://www.postgresql.org/message-id/TY3PR01MB9889C5D55206DDD978627D07F5752%40TY3PR01MB9889.jpnprd01.prod.outlook.com

Thanks and regards
Shlok Kyal

Dear hackers,

> We fixed some of the comments posted in the thread. We have created it
> as top-up patch 0002 and 0003.

I found that the CFbot raised an ERROR. Also, it may not work well in case
of production build.
PSA the fixed patch set.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Jan 22, 2024 at 2:38 PM Hayato Kuroda (Fujitsu)

<kuroda.hayato@fujitsu.com> wrote:

> >

> > > Yet other options could be

> > > pg_buildsubscriber, pg_makesubscriber as 'build' or 'make' in the name

> > > sounds like we are doing some work to create the subscriber which I

> > > think is the case here.

> >

> > I see your point here.  pg_createsubscriber is not like createuser in

> > that it just runs an SQL command.  It does something different than

> > CREATE SUBSCRIBER.

Right.

  So a different verb would make that clearer.  Maybe

> > something from here: https://www.thesaurus.com/browse/convert

>

> I read the link and found a good verb "switch". So, how about using "pg_switchsubscriber"?

>

I also initially thought on these lines and came up with a name like

pg_convertsubscriber but didn't feel strongly about it as that would

have sounded meaningful if we use a name like

pg_convertstandbytosubscriber. Now, that has become too long. Having

said that, I am not opposed to it having a name on those lines. BTW,

another option that occurred to me today is pg_preparesubscriber. We

internally create slots and then wait for wal, etc. which makes me

sound like adding 'prepare' in the name can also explain the purpose.

I analyzed and found a reason. This is because publications are invisible for some transactions.

As the first place, below operations were executed in this case.

Tuples were inserted after getting consistent_lsn, but before starting the standby.

After doing the workload, I confirmed again that the publication was created.

1. on primary, logical replication slots were created.

2. on primary, another replication slot was created.

3. ===on primary, some tuples were inserted. ===

4. on standby, a server process was started

5. on standby, the process waited until all changes have come.

6. on primary, publications were created.

7. on standby, subscriptions were created.

8. on standby, a replication progress for each subscriptions was set to given LSN (got at step2).

=====pg_subscriber finished here=====

9. on standby, a server process was started again

10. on standby, subscriptions were enabled. They referred slots created at step1.

11. on primary, decoding was started but ERROR was raised.

In this case, tuples were inserted *before creating publication*.

So I thought that the decoded transaction could not see the publication because

it was committed after insertions.

One solution is to create a publication before creating a consistent slot.

Changes which came before creating the slot were surely replicated to the standby,

so upcoming transactions can see the object. We are planning to patch set to fix

the issue in this approach.

We fixed some of the comments posted in the thread. We have created it

as top-up patch 0002 and 0003.

0002 patch contains the following changes:

* Add a timeout option for the recovery option, per [1]. The code was

basically ported from pg_ctl.c.

* Reject if the target server is not a standby, per [2]

* Raise FATAL error if --subscriber-conninfo specifies non-local server, per [3]

  (not sure it is really needed, so feel free reject the part.)

* Add check for max_replication_slots and wal_level; as per [4]

* Add -u and -p options; as per [5]

* Addressed comment except 5 and 8 in [6] and comment in [7]

0003 patch contains fix for bug reported in [8].

On Tue, Jan 23, 2024 at 7:41 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear hackers,
>
> > We fixed some of the comments posted in the thread. We have created it
> > as top-up patch 0002 and 0003.
>
> I found that the CFbot raised an ERROR. Also, it may not work well in case
> of production build.
> PSA the fixed patch set.

 Segmentation fault was found after testing the given command(There is
an extra '/' between 'new_standby2' and '-P') '$ gdb --args
./pg_subscriber -D ../new_standby2 / -P "host=localhost
port=5432 dbname=postgres" -d postgres'
While executing the above command, I got the following error:
pg_subscriber: error: too many command-line arguments (first is "/")
pg_subscriber: hint: Try "pg_subscriber --help" for more information.

Program received signal SIGSEGV, Segmentation fault.
0x0000555555557e5b in cleanup_objects_atexit () at pg_subscriber.c:173
173            if (perdb->made_subscription)
(gdb) p perdb
$1 = (LogicalRepPerdbInfo *) 0x0

Thanks and Regards,
Shubham Khanna.

Dear Shubham,

>  Segmentation fault was found after testing the given command(There is
> an extra '/' between 'new_standby2' and '-P') '$ gdb --args
> ./pg_subscriber -D ../new_standby2 / -P "host=localhost
> port=5432 dbname=postgres" -d postgres'
> While executing the above command, I got the following error:
> pg_subscriber: error: too many command-line arguments (first is "/")
> pg_subscriber: hint: Try "pg_subscriber --help" for more information.
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000555555557e5b in cleanup_objects_atexit () at pg_subscriber.c:173
> 173            if (perdb->made_subscription)
> (gdb) p perdb
> $1 = (LogicalRepPerdbInfo *) 0x0
>

Good catch, I could reproduce the issue. This crash was occurred because the
cleanup function was called before initialization memory.

There are several ways to fix it, but I chose to move the callback registration
behind. The function does actual tasks only after database objects are created.
So 0004 registers the function just before doing them. The memory allocation has
been done at that time. If required, Assert() can be added in the callback.

Can you test it and confirm the issue was solved?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear hackers,

Based on the requirement, I have profiled the performance test. It showed bottlenecks
are in small-data case are mainly two - starting a server and waiting until the
recovery is done.

# Tested source code

V7 patch set was applied atop HEAD(0eb23285). No configure options were specified
when it was built.

# Tested workload

I focused on only 100MB/1GB cases because bigger ones have already had good performance.
(Number of inserted tuples were same as previous tests)
I used bash script instead of tap test framework. See attached. Executed SQLs and
operations were almost the same.

As you can see, I tested only one-db case. Results may be changed if the number
of databases were changed.

# Measurement
Some debug logs which output current time were added (please see diff file).
I picked up some events and done at before/after them. Below bullets showed the measured ones:

* Starting a server
* Stopping a server
* Creating replication slots
* Creating publications
* Waiting until the recovery ended
* Creating subscriptions

# Result 
Below table shows the elapsed time for these events. Raw data is also available
by the attached excel file.

|Event category                  |100MB case [sec]|1GB [sec]|
|Starting a server               |1.414           |1.417    |
|Stoping a server                |0.506           |0.506    |
|Creating replication slots      |0.005           |0.007    |
|Creating publications           |0.001           |0.002    |
|Waiting until the recovery ended|1.603           |14.529   |
|Creating subscriptions          |0.012           |0.012    |
|Total                           |3.541           |16.473   |
|actual time                     |4.37            |17.271   |


As you can see, starting servers and waiting seem slow. We cannot omit these,
but setting smaller shared_buffers will reduce the start time. One approach is
to overwrite the GUC to smaller value, but I think we cannot determine the
appropriate value.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear hackers,

Here are comments for v8 patch set. I may revise them by myself,
but I want to post here to share all of them.

01.
```
/* Options */
static char *pub_conninfo_str = NULL;
static SimpleStringList database_names = {NULL, NULL};
static int    wait_seconds = DEFAULT_WAIT;
static bool retain = false;
static bool dry_run = false;
```

Just to confirm - is there a policy why we store the specified options? If you
want to store as global ones, username and port should follow (my fault...).
Or, should we have a structure to store them?

02.
```
{"subscriber-conninfo", required_argument, NULL, 'S'},
```

This is my fault, but "--subscriber-conninfo" is still remained. It should be
removed if it is not really needed.

03.
```
{"username", required_argument, NULL, 'u'},
```

Should we accept 'U' instead of 'u'?

04.
```
{"dry-run", no_argument, NULL, 'n'},
```

I'm not sure why the dry_run mode exists. In terms pg_resetwal, it shows the
which value would be changed based on the input. As for the pg_upgrade, it checks
whether the node can be upgraded for now. I think, we should have the checking
feature, so it should be renamed to --check. Also, the process should exit earlier
at that time.

05.
I felt we should accept some settings from enviroment variables, like pg_upgrade.
Currently, below items should be acceted.

- data directory
- username
- port
- timeout

06.
```
pg_logging_set_level(PG_LOG_WARNING);
```

If the default log level is warning, there are no ways to output debug logs.
(-v option only raises one, so INFO would be output)
I think it should be PG_LOG_INFO.

07.
Can we combine verifications into two functions, e.g., check_primary() and check_standby/check_subscriber()?

08.
Not sure, but if we want to record outputs by pg_subscriber, the sub-directory
should be created. The name should contain the timestamp.

09.
Not sure, but should we check max_slot_wal_keep_size of primary server? It can
avoid to fail starting of logical replicaiton.

10.
```
    nslots_new = nslots_old + dbarr.ndbs;

    if (nslots_new > max_replication_slots)
    {
        pg_log_error("max_replication_slots (%d) must be greater than or equal to "
                     "the number of replication slots required (%d)", max_replication_slots, nslots_new);
        exit(1);
    }
```

I think standby server must not have replication slots. Because subsequent
pg_resetwal command discards all the WAL file, so WAL records pointed by them
are removed. Currently pg_resetwal does not raise ERROR at that time.

11.
```
    /*
     * Stop the subscriber if it is a standby server. Before executing the
     * transformation steps, make sure the subscriber is not running because
     * one of the steps is to modify some recovery parameters that require a
     * restart.
     */
    if (stat(pidfile, &statbuf) == 0)
```

I kept just in case, but I'm not sure it is still needed. How do you think?
Removing it can reduce an inclusion of pidfile.h.

12.
```
        pg_ctl_cmd = psprintf("\"%s/pg_ctl\" stop -D \"%s\" -s",
                              standby.bindir, standby.pgdata);
        rc = system(pg_ctl_cmd);
        pg_ctl_status(pg_ctl_cmd, rc, 0);
```


There are two places to stop the instance. Can you divide it into a function?

13.
```
     * A temporary replication slot is not used here to avoid keeping a
     * replication connection open (depending when base backup was taken, the
     * connection should be open for a few hours).
     */
    conn = connect_database(primary.base_conninfo, dbarr.perdb[0].dbname);
    if (conn == NULL)
        exit(1);
    consistent_lsn = create_logical_replication_slot(conn, true,
                                                     &dbarr.perdb[0]);
```

I didn't notice the comment, but still not sure the reason. Why we must reserve
the slot until pg_subscriber finishes? IIUC, the slot would be never used, it
is created only for getting a consistent_lsn. So we do not have to keep.
Also, just before, logical replication slots for each databases are created, so
WAL records are surely reserved.

14.

```
    pg_log_info("starting the subscriber");
    start_standby_server(&standby, subport, server_start_log);
```

This info should be in the function.

15.
```
    /*
     * Create a subscription for each database.
     */
    for (i = 0; i < dbarr.ndbs; i++)
```

This can be divided into a function, like create_all_subscriptions().

16.
My fault: usage() must be updated.

17. use_primary_slot_name
```
    if (PQntuples(res) != 1)
    {
        pg_log_error("could not obtain replication slot information: got %d rows, expected %d row",
                     PQntuples(res), 1);
        return NULL;
    }
```

Error message should be changed. I think this error means the standby has wrong primary_slot_name, right?

18. misc
Sometimes the pid of pg_subscriber is referred. It can be stored as global variable.

19.
C99-style has been allowed, so loop variables like "i" can be declared in the for-statement, like

```
for (int i = 0; i < MAX; i++)
```

20.
Some comments, docs, and outputs must be fixed when the name is changed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On 24.01.24 00:44, Euler Taveira wrote:
> Subscriber has a different meaning of subscription. Subscription is an SQL
> object. Subscriber is the server (node in replication terminology) where the
> subscription resides. Having said that pg_createsubscriber doesn't seem 
> a bad
> name because you are creating a new subscriber. (Indeed, you are 
> transforming /
> converting but "create" seems closer and users can infer that it is a 
> tool to
> build a new logical replica.

That makes sense.

(Also, the problem with "convert" etc. is that "convertsubscriber" would 
imply that you are converting an existing subscriber to something else. 
It would need to be something like "convertbackup" then, which doesn't 
seem helpful.)

> I think "convert" and "transform" fit for this case. However, "create",
> "convert" and "transform" have 6, 7 and 9 characters,  respectively. I 
> suggest
> that we avoid long names (subscriber already has 10 characters). My 
> preference
> is pg_createsubscriber.

That seems best to me.

Dear Peter,

Thanks for giving your idea!

> > Subscriber has a different meaning of subscription. Subscription is an SQL
> > object. Subscriber is the server (node in replication terminology) where the
> > subscription resides. Having said that pg_createsubscriber doesn't seem
> > a bad
> > name because you are creating a new subscriber. (Indeed, you are
> > transforming /
> > converting but "create" seems closer and users can infer that it is a
> > tool to
> > build a new logical replica.
> 
> That makes sense.
> 
> (Also, the problem with "convert" etc. is that "convertsubscriber" would
> imply that you are converting an existing subscriber to something else.
> It would need to be something like "convertbackup" then, which doesn't
> seem helpful.)
> 
> > I think "convert" and "transform" fit for this case. However, "create",
> > "convert" and "transform" have 6, 7 and 9 characters,  respectively. I
> > suggest
> > that we avoid long names (subscriber already has 10 characters). My
> > preference
> > is pg_createsubscriber.
> 
> That seems best to me.

Just FYI - I'm ok to change the name to pg_createsubscriber.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

I'll post a new one soon.

01.

```

/* Options */

static char *pub_conninfo_str = NULL;

static SimpleStringList database_names = {NULL, NULL};

static int wait_seconds = DEFAULT_WAIT;

static bool retain = false;

static bool dry_run = false;

```

Just to confirm - is there a policy why we store the specified options? If you

want to store as global ones, username and port should follow (my fault...).

Or, should we have a structure to store them?

04.

```

{"dry-run", no_argument, NULL, 'n'},

```

I'm not sure why the dry_run mode exists. In terms pg_resetwal, it shows the

which value would be changed based on the input. As for the pg_upgrade, it checks

whether the node can be upgraded for now. I think, we should have the checking

feature, so it should be renamed to --check. Also, the process should exit earlier

at that time.

05.

I felt we should accept some settings from enviroment variables, like pg_upgrade.

Currently, below items should be acceted.

- data directory

- username

- port

- timeout

06.

```

pg_logging_set_level(PG_LOG_WARNING);

```

If the default log level is warning, there are no ways to output debug logs.

(-v option only raises one, so INFO would be output)

I think it should be PG_LOG_INFO.

07.

Can we combine verifications into two functions, e.g., check_primary() and check_standby/check_subscriber()?

08.

Not sure, but if we want to record outputs by pg_subscriber, the sub-directory

should be created. The name should contain the timestamp.

09.

Not sure, but should we check max_slot_wal_keep_size of primary server? It can

avoid to fail starting of logical replicaiton.

10.

```

nslots_new = nslots_old + dbarr.ndbs;

if (nslots_new > max_replication_slots)

{

pg_log_error("max_replication_slots (%d) must be greater than or equal to "

"the number of replication slots required (%d)", max_replication_slots, nslots_new);

exit(1);

}

```

I think standby server must not have replication slots. Because subsequent

pg_resetwal command discards all the WAL file, so WAL records pointed by them

are removed. Currently pg_resetwal does not raise ERROR at that time.

11.

```

/*

* Stop the subscriber if it is a standby server. Before executing the

* transformation steps, make sure the subscriber is not running because

* one of the steps is to modify some recovery parameters that require a

* restart.

*/

if (stat(pidfile, &statbuf) == 0)

```

I kept just in case, but I'm not sure it is still needed. How do you think?

Removing it can reduce an inclusion of pidfile.h.

12.

```

pg_ctl_cmd = psprintf("\"%s/pg_ctl\" stop -D \"%s\" -s",

  standby.bindir, standby.pgdata);

rc = system(pg_ctl_cmd);

pg_ctl_status(pg_ctl_cmd, rc, 0);

```

There are two places to stop the instance. Can you divide it into a function?

13.

```

* A temporary replication slot is not used here to avoid keeping a

* replication connection open (depending when base backup was taken, the

* connection should be open for a few hours).

*/

conn = connect_database(primary.base_conninfo, dbarr.perdb[0].dbname);

if (conn == NULL)

exit(1);

consistent_lsn = create_logical_replication_slot(conn, true,

&dbarr.perdb[0]);

```

I didn't notice the comment, but still not sure the reason. Why we must reserve

the slot until pg_subscriber finishes? IIUC, the slot would be never used, it

is created only for getting a consistent_lsn. So we do not have to keep.

Also, just before, logical replication slots for each databases are created, so

WAL records are surely reserved.

14.

```

pg_log_info("starting the subscriber");

start_standby_server(&standby, subport, server_start_log);

```

This info should be in the function.

15.

```

/*

* Create a subscription for each database.

*/

for (i = 0; i < dbarr.ndbs; i++)

```

This can be divided into a function, like create_all_subscriptions().

16.

My fault: usage() must be updated.

17. use_primary_slot_name

```

if (PQntuples(res) != 1)

{

pg_log_error("could not obtain replication slot information: got %d rows, expected %d row",

PQntuples(res), 1);

return NULL;

}

```

Error message should be changed. I think this error means the standby has wrong primary_slot_name, right?

18. misc

Sometimes the pid of pg_subscriber is referred. It can be stored as global variable.

19.

C99-style has been allowed, so loop variables like "i" can be declared in the for-statement, like

```

for (int i = 0; i < MAX; i++)

```

20.

Some comments, docs, and outputs must be fixed when the name is changed.

Dear Euler,

Thanks for updating the patch! Before reading yours, I wanted to reply some of comments.

>
I'm still thinking about replacing --subscriber-conninfo with separate items
(username, port, password?, host = socket dir). Maybe it is an overengineering.
The user can always prepare the environment to avoid unwanted and/or external
connections.
>

For me, required amount of fixes are not so different from current one. How about
others?

>
It is extremely useful because (a) you have a physical replication setup and
don't know if it is prepared for logical replication, (b) check GUCs (is
max_wal_senders sufficient for this pg_subscriber command? Or is
max_replication_slots sufficient to setup the logical replication even though I
already have some used replication slots?), (c) connectivity and (d)
credentials.
>

Yeah, it is useful for verification purpose, so let's keep this option.
But I still think the naming should be "--check". Also, there are many
`if (!dry_run)` but most of them can be removed if the process exits earlier.
Thought?


>
> 05.
> I felt we should accept some settings from enviroment variables, like pg_upgrade.
> Currently, below items should be acceted.
>
> - data directory
> - username
> - port
> - timeout

Maybe PGDATA.
>

Sorry, I cannot follow this. Did you mean that the target data directory should
be able to be specified by PGDATA? OF so, +1.

>
> 06.
> ```
> pg_logging_set_level(PG_LOG_WARNING);
> ```
>
> If the default log level is warning, there are no ways to output debug logs.
> (-v option only raises one, so INFO would be output)
> I think it should be PG_LOG_INFO.

You need to specify multiple -v options.
>

Hmm. I felt the specification was bit strange...but at least it must be
described on the documentation. pg_dump.sgml has similar lines.

>
> 08.
> Not sure, but if we want to record outputs by pg_subscriber, the sub-directory
> should be created. The name should contain the timestamp.

The log file already contains the timestamp. Why?
>

This comment assumed outputs by pg_subscriber were also recorded to a file.
In this case and if the file also has the same timestamp, I think they can be
gathered in the same place. No need if outputs are not recorded.


>
> 09.
> Not sure, but should we check max_slot_wal_keep_size of primary server? It can
> avoid to fail starting of logical replicaiton.

A broken physical replication *before* running this tool is its responsibility?
Hmm. We might add another check that can be noticed during dry run mode.
>

I thought that we should not generate any broken objects, but indeed, not sure
it is our scope. How do other think?


>
> 11.
> ```
> /*
> * Stop the subscriber if it is a standby server. Before executing the
> * transformation steps, make sure the subscriber is not running because
> * one of the steps is to modify some recovery parameters that require a
> * restart.
> */
> if (stat(pidfile, &statbuf) == 0)
> ```
>
> I kept just in case, but I'm not sure it is still needed. How do you think?
> Removing it can reduce an inclusion of pidfile.h.

Are you suggesting another way to check if the standby is up and running?
>

Running `pg_ctl stop` itself can detect whether the process has been still alive.
It would exit with 1 when the process is not there.

>
> I didn't notice the comment, but still not sure the reason. Why we must reserve
> the slot until pg_subscriber finishes? IIUC, the slot would be never used, it
> is created only for getting a consistent_lsn. So we do not have to keep.
> Also, just before, logical replication slots for each databases are created, so
> WAL records are surely reserved.
>

I want to confirm the conclusion - will you remove the creation of a transient slot?

Also, not tested, I'm now considering that we can reuse the primary_conninfo value.
We are assuming that the target server is standby and the current upstream one will
convert to publisher. In this case, the connection string is already specified as
primary_conninfo so --publisher-conninfo may not be needed. The parameter does
not contain database name, so --databases is still needed. I imagine like:

1. Parse options
2. Turn on standby
3. Verify the standby
4. Turn off standby
5. Get primary_conninfo from standby
6. Connect to primary (concat of primary_conninfo and an option is used)
7. Verify the primary
...

How do you think?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Dear Euler,

Again, thanks for updating the patch! There are my random comments for v9.

01.
I cannot find your replies for my comments#7 [1] but you reverted related changes.
I'm not sure you are still considering it or you decided not to include changes.
Can you clarify your opinion?
(It is needed because changes are huge so it quite affects other developments...)

02.
```
+       <term><option>-t <replaceable class="parameter">seconds</replaceable></option></term>
+       <term><option>--timeout=<replaceable class="parameter">seconds</replaceable></option></term>
```

But source codes required `--recovery-timeout`. Please update either of them,

03.
```
+ *    Create a new logical replica from a standby server
```

Junwang pointed out to change here but the change was reverted [2]
Can you clarify your opinion as well?

04.
```
+/*
+ * Is the source server ready for logical replication? If so, create the
+ * publications and replication slots in preparation for logical replication.
+ */
+static bool
+setup_publisher(LogicalRepInfo *dbinfo)
```

But this function verifies the source server. I felt they should be in the
different function.

05.
```
+/*
+ * Is the target server ready for logical replication?
+ */
+static bool
+setup_subscriber(LogicalRepInfo *dbinfo)
````

Actually, this function does not set up subscriber. It just verifies whether the
target can become a subscriber, right? If should be renamed.

06.
```
+    atexit(cleanup_objects_atexit);
```

The registration of the cleanup function is too early. This sometimes triggers
a core-dump. E.g.,

```
$ pg_subscriber --publisher-conninfo --subscriber-conninfo 'user=postgres port=5432' --verbose --database 'postgres'
--pgdatadata_N2/ 
pg_subscriber: error: too many command-line arguments (first is "user=postgres port=5432")
pg_subscriber: hint: Try "pg_subscriber --help" for more information.
Segmentation fault (core dumped)

$ gdb ...
(gdb) bt
#0  cleanup_objects_atexit () at pg_subscriber.c:131
#1  0x00007fb982cffce9 in __run_exit_handlers () from /lib64/libc.so.6
#2  0x00007fb982cffd37 in exit () from /lib64/libc.so.6
#3  0x00000000004054e6 in main (argc=9, argv=0x7ffc59074158) at pg_subscriber.c:1500
(gdb) f 3
#3  0x00000000004054e6 in main (argc=9, argv=0x7ffc59074158) at pg_subscriber.c:1500
1500                    exit(1);
(gdb) list
1495            if (optind < argc)
1496            {
1497                    pg_log_error("too many command-line arguments (first is \"%s\")",
1498                                             argv[optind]);
1499                    pg_log_error_hint("Try \"%s --help\" for more information.", progname);
1500                    exit(1);
1501            }
1502
1503            /*
1504             * Required arguments
```

I still think it should be done just before the creation of objects [3].

07.
Missing a removal of publications on the standby.

08.
Missing registration of LogicalRepInfo in the typedefs.list.

09
```
+ <refsynopsisdiv>
+  <cmdsynopsis>
+   <command>pg_subscriber</command>
+   <arg rep="repeat"><replaceable>option</replaceable></arg>
+  </cmdsynopsis>
+ </refsynopsisdiv>
```

Can you reply my comment#2 [4]? I think mandatory options should be written.

10.
Just to confirm - will you implement start_standby/stop_standby functions in next version?

[1]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[2]: https://www.postgresql.org/message-id/CAEG8a3%2BwL_2R8n12BmRz7yBP3EBNdHDhmdgxQFA9vS%2BzPR%2B3Kw%40mail.gmail.com
[3]:
https://www.postgresql.org/message-id/TY3PR01MB9889678E47B918F4D83A6FD8F57B2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[4]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Again, thanks for updating the patch! There are my random comments for v9.

01.

I cannot find your replies for my comments#7 [1] but you reverted related changes.

I'm not sure you are still considering it or you decided not to include changes.

Can you clarify your opinion?

(It is needed because changes are huge so it quite affects other developments...)

02.

```

+       <term><option>-t <replaceable class="parameter">seconds</replaceable></option></term>

+       <term><option>--timeout=<replaceable class="parameter">seconds</replaceable></option></term>

```

But source codes required `--recovery-timeout`. Please update either of them,

03.

```

+ *    Create a new logical replica from a standby server

```

Junwang pointed out to change here but the change was reverted [2]

Can you clarify your opinion as well?

04.

```

+/*

+ * Is the source server ready for logical replication? If so, create the

+ * publications and replication slots in preparation for logical replication.

+ */

+static bool

+setup_publisher(LogicalRepInfo *dbinfo)

```

But this function verifies the source server. I felt they should be in the

different function.

05.

```

+/*

+ * Is the target server ready for logical replication?

+ */

+static bool

+setup_subscriber(LogicalRepInfo *dbinfo)

````

Actually, this function does not set up subscriber. It just verifies whether the

target can become a subscriber, right? If should be renamed.

06.

```

+    atexit(cleanup_objects_atexit);

```

The registration of the cleanup function is too early. This sometimes triggers

a core-dump. E.g., 

07.

Missing a removal of publications on the standby.

08.

Missing registration of LogicalRepInfo in the typedefs.list.

09

```

+ <refsynopsisdiv>

+  <cmdsynopsis>

+   <command>pg_subscriber</command>

+   <arg rep="repeat"><replaceable>option</replaceable></arg>

+  </cmdsynopsis>

+ </refsynopsisdiv>

```

Can you reply my comment#2 [4]? I think mandatory options should be written.

10.

Just to confirm - will you implement start_standby/stop_standby functions in next version?

Dear Euler,

>
It is still on my list. As I said in a previous email I'm having a hard time
reviewing pieces from your 0002 patch because you include a bunch of things
...
It is still on my list.
>

I understood that patches we posted were bad. Sorry for inconvenience.
So I extracted changes only related with them. Can you review them and include
If it seems OK?

v10-0001: same as v9-0001.
0002: not related with our changes, but this fixed a small bug.
          The third argument of getopt_long was not correct.
0003: adds start_standby/stop_standby funcitons
0004: tries to refactor some structures.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

On Fri, Jan 26, 2024, at 4:55 AM, Hayato Kuroda (Fujitsu) wrote:

Again, thanks for updating the patch! There are my random comments for v9.

Thanks for checking v9. I already incorporated some of the points below into

the next patch. Give me a couple of hours to include some important points.

Dear Euler,

Thanks for updating the patch!

> One open item that is worrying me is how to handle the pg_ctl timeout. This
> patch does nothing and the user should use PGCTLTIMEOUT environment variable to
> avoid that the execution is canceled after 60 seconds (default for pg_ctl).
> Even if you set a high value, it might not be enough for cases like
> time-delayed replica. Maybe pg_ctl should accept no timeout as --timeout
> option. I'll include this caveat into the documentation but I'm afraid it is
> not sufficient and we should provide a better way to handle this situation.

I felt you might be confused a bit. Even if the recovery_min_apply_delay is set,
e.g., 10h., the pg_ctl can start and stop the server. This is because the
walreceiver serialize changes as soon as they received. The delay is done by the
startup process. There are no unflushed data, so server instance can be turned off.
If you meant the combination of recovery-timeout and time-delayed replica, yes,
it would be likely to occur. But in the case, using like --no-timeout option is
dangerous. I think we should overwrite recovery_min_apply_delay to zero. Thought?

Below part contains my comments for v11-0001. Note that the ordering is random.

01. doc
```
    <group choice="req">
     <arg choice="plain"><option>-D</option> </arg>
     <arg choice="plain"><option>--pgdata</option></arg>
    </group>
```

According to other documentation like pg_upgrade, we do not write both longer
and shorter options in the synopsis section.

02. doc
```
  <para>
   <application>pg_createsubscriber</application> takes the publisher and subscriber
   connection strings, a cluster directory from a physical replica and a list of
   database names and it sets up a new logical replica using the physical
   recovery process.
  </para>

```

I found that you did not include my suggestion without saying [1]. Do you dislike
the comment or still considering?

03. doc
```
      <term><option>-P  <replaceable class="parameter">connstr</replaceable></option></term>
```

Too many blank after -P.

04. doc
```
    <para>
     <application>pg_createsubscriber</application> checks if the target data
     directory is used by a physical replica. Stop the physical replica if it is
     running. One of the next steps is to add some recovery parameters that
     requires a server start. This step avoids an error.
    </para>
```

I think doc is not updated. Currently (not sure it is good) the physical replica
must be running before doing pg_createsubscriber.

05. doc
```
     each specified database on the source server. The replication slot name
     contains a <literal>pg_createsubscriber</literal> prefix. These replication
     slots will be used by the subscriptions in a future step.  A temporary
```

According to the "Replication Slot Management" subsection in logical-replication.sgml,
the format of names should be described expressly, e.g.:

```
These replication slots have generated names:"pg_createsubscriber_%u_%d" (parameters: Subscription oid, Process id
pid).
```

Publications, a temporary slot, and subscriptions should be described as well.

06. doc
```
     Next, <application>pg_createsubscriber</application> creates one publication
     for each specified database on the source server. Each publication
     replicates changes for all tables in the database. The publication name

```

Missing update. Publications are created before creating replication slots.

07. general
I think there are some commenting conversions in PG, but this file breaks it.

* Comments must be one-line as much as possible
* Periods must not be added when the comment is one-line.
* Initial character must be capital.

08. general
Some pg_log_error() + exit(1) can be replaced with pg_fatal().


09. LogicalRepInfo
```
    char       *subconninfo;    /* subscription connection string for logical
                                 * replication */
```

As I posted in comment#8[2], I don't think it is "subscription connection". Also,
"for logical replication" is bit misreading because it would not be passed to
workers.

10. get_base_conninfo
```
static char *
get_base_conninfo(char *conninfo, char *dbname, const char *noderole)
...
        /*
         * If --database option is not provided, try to obtain the dbname from
         * the publisher conninfo. If dbname parameter is not available, error
         * out.
         */

```

I'm not sure getting dbname from the conninfo improves user-experience. I felt
it may trigger an unintended targeting.
(I still think the publisher-server should be removed)

11. check_data_directory
```
/*
 * Is it a cluster directory? These are preliminary checks. It is far from
 * making an accurate check. If it is not a clone from the publisher, it will
 * eventually fail in a future step.
 */
static bool
check_data_directory(const char *datadir)
```

We shoud also check whether pg_createsubscriber can create a file and a directory.
GetDataDirectoryCreatePerm() verifies it.

12. main
```
    /* Register a function to clean up objects in case of failure. */
    atexit(cleanup_objects_atexit);
```

According to the manpage, callback functions would not be called when it exits
due to signals:

> Functions  registered  using atexit() (and on_exit(3)) are not called if a
> process terminates abnormally because of the delivery of a signal.

Do you have a good way to handle the case? One solution is to output created
objects in any log level, but the consideration may be too much. Thought?

13, main
```
    /*
     * Create a temporary logical replication slot to get a consistent LSN.
```

Just to clarify - I still think the process exits before here in case of dry run.
In case of pg_resetwal, the process exits before doing actual works like
RewriteControlFile().

14. main
```
     * XXX we might not fail here. Instead, we provide a warning so the user
     * eventually drops this replication slot later.
```

But there are possibilities to exit(1) in drop_replication_slot(). Is it acceptable?

15. wait_for_end_recovery
```
        /*
         * Bail out after recovery_timeout seconds if this option is set.
         */
        if (recovery_timeout > 0 && timer >= recovery_timeout)
```

Hmm, IIUC, it should be enabled by default [3]. Do you have anything in your mind?

16. main
```
    /*
     * Create the subscription for each database on subscriber. It does not
     * enable it immediately because it needs to adjust the logical
     * replication start point to the LSN reported by consistent_lsn (see
     * set_replication_progress). It also cleans up publications created by
     * this tool and replication to the standby.
     */
    if (!setup_subscriber(dbinfo, consistent_lsn))
```

Subscriptions would be created and replication origin would be moved forward here,
but latter one can be done only by the superuser. I felt that this should be
checked in check_subscriber().

17. main
```
    /*
     * Change system identifier.
     */
    modify_sysid(pg_resetwal_path, subscriber_dir);
```

Even if I executed without -v option, an output from pg_resetwal command appears.
It seems bit strange.

```
$ pg_createsubscriber -D data_N2/ -P "port=5431 user=postgres" -S "port=5432 user=postgres" -d postgres
Write-ahead log reset
$
```

[1]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/message-id/CAA4eK1JRgnhv_ySzuFjN7UaX9qxz5Hqcwew7Fk%3D%2BAbJbu0Kd9w%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Dear Euler,

I extracted some review comments which may require many efforts. I hope this makes them
easy to review.

0001: not changed from yours.
0002: avoid to use replication connections. Source: comment #3[1]
0003: Remove -P option and use primary_conninfo instead. Source: [2]
0004: Exit earlier when dry_run is specified. Source: [3]
0005: Refactor data structures. Source: [4]

[1]:
https://www.postgresql.org/message-id/TY3PR01MB9889593399165B9A04106741F5662%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/TY3PR01MB98897C85700C6DF942D2D0A3F5792%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[3]:
https://www.postgresql.org/message-id/TY3PR01MB98897C85700C6DF942D2D0A3F5792%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[4]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

Jumping into this a bit late here... I'm trying a simple pg_createsubscriber but getting an error:

Seems we need to escape connection params similar we do in dblink [1]

On Wed, Jan 31, 2024 at 11:35 AM Euler Taveira <euler@eulerto.com> wrote:

>

> On Wed, Jan 31, 2024, at 11:25 AM, Fabrízio de Royes Mello wrote:

>

> Jumping into this a bit late here... I'm trying a simple pg_createsubscriber but getting an error:

>

>

> Try v11. It seems v12-0002 is not correct.

Using v11 I'm getting this error:

~/pgsql took 22s

✦ ➜ pg_createsubscriber -d fabrizio -r -D /tmp/replica5434 -S 'host=/tmp port=5434' -P 'host=/tmp port=5432'

NOTICE:  changed the failover state of replication slot "pg_createsubscriber_16384_706609" on publisher to false

pg_createsubscriber: error: could not drop replication slot "pg_createsubscriber_706609_startpoint" on database "fabrizio": ERROR:  replication slot "pg_createsubscriber_706609_startpoint" does not exist

Write-ahead log reset

> One open item that is worrying me is how to handle the pg_ctl timeout. This

> patch does nothing and the user should use PGCTLTIMEOUT environment variable to

> avoid that the execution is canceled after 60 seconds (default for pg_ctl).

> Even if you set a high value, it might not be enough for cases like

> time-delayed replica. Maybe pg_ctl should accept no timeout as --timeout

> option. I'll include this caveat into the documentation but I'm afraid it is

> not sufficient and we should provide a better way to handle this situation.

I felt you might be confused a bit. Even if the recovery_min_apply_delay is set,

e.g., 10h., the pg_ctl can start and stop the server. This is because the

walreceiver serialize changes as soon as they received. The delay is done by the

startup process. There are no unflushed data, so server instance can be turned off.

If you meant the combination of recovery-timeout and time-delayed replica, yes,

it would be likely to occur. But in the case, using like --no-timeout option is

dangerous. I think we should overwrite recovery_min_apply_delay to zero. Thought?

Below part contains my comments for v11-0001. Note that the ordering is random.

01. doc

```

    <group choice="req">

     <arg choice="plain"><option>-D</option> </arg>

     <arg choice="plain"><option>--pgdata</option></arg>

    </group>

```

According to other documentation like pg_upgrade, we do not write both longer

and shorter options in the synopsis section.

02. doc

```

  <para>

   <application>pg_createsubscriber</application> takes the publisher and subscriber

   connection strings, a cluster directory from a physical replica and a list of

   database names and it sets up a new logical replica using the physical

   recovery process.

  </para>

```

I found that you did not include my suggestion without saying [1]. Do you dislike

the comment or still considering?

03. doc

```

      <term><option>-P  <replaceable class="parameter">connstr</replaceable></option></term>

```

Too many blank after -P.

07. general

I think there are some commenting conversions in PG, but this file breaks it.

08. general

Some pg_log_error() + exit(1) can be replaced with pg_fatal().

09. LogicalRepInfo

```

char    *subconninfo; /* subscription connection string for logical

* replication */

```

As I posted in comment#8[2], I don't think it is "subscription connection". Also,

"for logical replication" is bit misreading because it would not be passed to

workers.

10. get_base_conninfo

```

static char *

get_base_conninfo(char *conninfo, char *dbname, const char *noderole)

...

/*

* If --database option is not provided, try to obtain the dbname from

* the publisher conninfo. If dbname parameter is not available, error

* out.

*/

```

I'm not sure getting dbname from the conninfo improves user-experience. I felt

it may trigger an unintended targeting.

(I still think the publisher-server should be removed)

11. check_data_directory

```

/*

* Is it a cluster directory? These are preliminary checks. It is far from

* making an accurate check. If it is not a clone from the publisher, it will

* eventually fail in a future step.

*/

static bool

check_data_directory(const char *datadir)

```

We shoud also check whether pg_createsubscriber can create a file and a directory.

GetDataDirectoryCreatePerm() verifies it.

12. main

```

/* Register a function to clean up objects in case of failure. */

atexit(cleanup_objects_atexit);

```

According to the manpage, callback functions would not be called when it exits

due to signals:

> Functions  registered  using atexit() (and on_exit(3)) are not called if a

> process terminates abnormally because of the delivery of a signal.

Do you have a good way to handle the case? One solution is to output created

objects in any log level, but the consideration may be too much. Thought?

13, main

```

/*

* Create a temporary logical replication slot to get a consistent LSN.

```

Just to clarify - I still think the process exits before here in case of dry run.

In case of pg_resetwal, the process exits before doing actual works like

RewriteControlFile().

14. main

```

* XXX we might not fail here. Instead, we provide a warning so the user

* eventually drops this replication slot later.

```

But there are possibilities to exit(1) in drop_replication_slot(). Is it acceptable?

15. wait_for_end_recovery

```

/*

* Bail out after recovery_timeout seconds if this option is set.

*/

if (recovery_timeout > 0 && timer >= recovery_timeout)

```

Hmm, IIUC, it should be enabled by default [3]. Do you have anything in your mind?

16. main

```

/*

* Create the subscription for each database on subscriber. It does not

* enable it immediately because it needs to adjust the logical

* replication start point to the LSN reported by consistent_lsn (see

* set_replication_progress). It also cleans up publications created by

* this tool and replication to the standby.

*/

if (!setup_subscriber(dbinfo, consistent_lsn))

```

Subscriptions would be created and replication origin would be moved forward here,

but latter one can be done only by the superuser. I felt that this should be

checked in check_subscriber().

17. main

```

/*

* Change system identifier.

*/

modify_sysid(pg_resetwal_path, subscriber_dir);

```

Even if I executed without -v option, an output from pg_resetwal command appears.

It seems bit strange.

Dear Fabrízio,

Thanks for reporting. I understood that the issue occurred on v11 and v12.
I will try to reproduce and check the reason.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

Dear Euler,

Thanks for giving comments! I want to reply some of them.

>
I didn't provide the whole explanation. I'm envisioning the use case that pg_ctl
doesn't reach the consistent state and the timeout is reached (the consequence
is that pg_createsubscriber aborts the execution). It might occur on a busy
server. The probability that it occurs with the current code is low (LSN gap
for recovery is small). Maybe I'm anticipating issues when the base backup
support is added but better to raise concerns during development.
>

Hmm, actually I didn't know the case. Thanks for explanation. I want to see
how you describe on the doc.

>
pg_upgrade doesn't but others do like pg_rewind, pg_resetwal, pg_controldata,
pg_checksums. It seems newer tools tend to provide short and long options.
>

Oh, you are right.

>
Nothing? If you interrupt the execution, there will be objects left behind and
you, as someone that decided to do it, have to clean things up. What do you
expect this tool to do? The documentation will provide some guidance informing
the object name patterns this tool uses and you can check for leftover objects.
Someone can argue that is a valid feature request but IMO it is not one in the
top of the list.
>

OK, so let's keep current style.

>
Why? Are you suggesting that the dry run mode covers just the verification
part? If so, it is not a dry run mode. I would expect it to run until the end
(or until it accomplish its goal) but *does not* modify data. For pg_resetwal,
the modification is one of the last steps and the other ones (KillFoo
functions) that are skipped modify data. It ends the dry run mode when it
accomplish its goal (obtain the new control data values). If we stop earlier,
some of the additional steps won't be covered by the dry run mode and a failure
can happen but could be detected if you run a few more steps.
>

Yes, it was my expectation. I'm still not sure which operations can detect by the
dry_run, but we can keep it for now.

>
Why? See [1]. I prefer the kind mode (always wait until the recovery ends) but
you and Amit are proposing a more aggressive mode. The proposal (-t 60) seems
ok right now, however, if the goal is to provide base backup support in the
future, you certainly should have to add the --recovery-timeout in big clusters
or those with high workloads because base backup is run between replication slot
creation and consistent LSN. Of course, we can change the default when base
backup support is added.
>

Sorry, I was missing your previous post. Let's keep yours.

>
Good point. I included a check for pg_create_subscription role and CREATE
privilege on the specified database.
>

Not sure, but can we do the replication origin functions by these privilege?
According to the doc[1], these ones seem not to be related.

[1]: https://www.postgresql.org/docs/devel/functions-admin.html#FUNCTIONS-REPLICATION

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

>

Why? Are you suggesting that the dry run mode covers just the verification

part? If so, it is not a dry run mode. I would expect it to run until the end

(or until it accomplish its goal) but *does not* modify data. For pg_resetwal,

the modification is one of the last steps and the other ones (KillFoo

functions) that are skipped modify data. It ends the dry run mode when it

accomplish its goal (obtain the new control data values). If we stop earlier,

some of the additional steps won't be covered by the dry run mode and a failure

can happen but could be detected if you run a few more steps.

>

Yes, it was my expectation. I'm still not sure which operations can detect by the

dry_run, but we can keep it for now.

Good point. I included a check for pg_create_subscription role and CREATE

privilege on the specified database.

>

Not sure, but can we do the replication origin functions by these privilege?

According to the doc[1], these ones seem not to be related.

Dear Fabrízio, Euler,

I think you set the primary_slot_name to the standby server, right?
While reading codes, I found below line in v11-0001.
```
    if (primary_slot_name != NULL)
    {
        conn = connect_database(dbinfo[0].pubconninfo);
        if (conn != NULL)
        {
            drop_replication_slot(conn, &dbinfo[0], temp_replslot);
        }
```

Now the temp_replslot is temporary one, so it would be removed automatically.
This function will cause the error: replication slot "pg_createsubscriber_%u_startpoint" does not exist.
Also, the physical slot is still remained on the primary. 
In short, "temp_replslot" should be "primary_slot_name".

PSA a script file for reproducing.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear Fabrízio, Euler,

I made fix patches to solve reported issues by Fabrízio.

* v13-0001: Same as v11-0001 made by Euler.
* v13-0002: Fixes ERRORs while dropping replication slots [1].
           If you want to see codes which we get agreement, please apply until 0002.
=== experimental patches ===
* v13-0003: Avoids to use replication connections. The issue [2] was solved on my env.
* v13-0004: Removes -P option and use primary_conninfo instead.
* v13-0005: Refactors data structures

[1]: https://www.postgresql.org/message-id/CAFcNs%2BrSG9DcEewsoA%3D85DXhSRh%2BnyKrrcr64FEDytcZf6QaEQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAFcNs%2BpPtw%2By7Be00BK0MBpHhLk2s66tLM286g%3Dk5rew8kUxjg%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

I made fix patches to solve reported issues by Fabrízio.

* v13-0001: Same as v11-0001 made by Euler.

* v13-0002: Fixes ERRORs while dropping replication slots [1].

           If you want to see codes which we get agreement, please apply until 0002.

=== experimental patches ===

* v13-0003: Avoids to use replication connections. The issue [2] was solved on my env.

* v13-0004: Removes -P option and use primary_conninfo instead.

* v13-0005: Refactors data structures

Dear Euler,

Thanks for updating the patch!

>
I'm still working on the data structures to group options. I don't like the way
it was grouped in v13-0005. There is too many levels to reach database name.
The setup_subscriber() function requires the 3 data structures.
>

Right, your refactoring looks fewer stack. So I pause to revise my refactoring
patch.

>
The documentation update is almost there. I will include the modifications in
the next patch.
>

OK. I think it should be modified before native speakers will attend to the
thread.

>
Regarding v13-0004, it seems a good UI that's why I wrote a comment about it.
However, it comes with a restriction that requires a similar HBA rule for both
regular and replication connections. Is it an acceptable restriction? We might
paint ourselves into the corner. A reasonable proposal is not to remove this
option. Instead, it should be optional. If it is not provided, primary_conninfo
is used.
>

I didn't have such a point of view. However, it is not related whether -P exists
or not. Even v14-0001 requires primary to accept both normal/replication connections.
If we want to avoid it, the connection from pg_createsubscriber can be restored
to replication-connection.
(I felt we do not have to use replication protocol even if we change the connection mode)

The motivation why -P is not needed is to ensure the consistency of nodes.
pg_createsubscriber assumes that the -P option can connect to the upstream node,
but no one checks it. Parsing two connection strings may be a solution but be
confusing. E.g., what if some options are different?
I think using a same parameter is a simplest solution.

And below part contains my comments for v14.

01.
```
char        temp_replslot[NAMEDATALEN] = {0};
```

I found that no one refers the name of temporary slot. Can we remove the variable?

02.
```
    CreateSubscriberOptions opt;
...
    memset(&opt, 0, sizeof(CreateSubscriberOptions));

    /* Default settings */
    opt.subscriber_dir = NULL;
    opt.pub_conninfo_str = NULL;
    opt.sub_conninfo_str = NULL;
    opt.database_names = (SimpleStringList)
    {
        NULL, NULL
    };
    opt.retain = false;
    opt.recovery_timeout = 0;
```

Initialization by `CreateSubscriberOptions opt = {0};` seems enough.
All values are set to 0x0.

03.
```
/*
 * Is the standby server ready for logical replication?
 */
static bool
check_subscriber(LogicalRepInfo *dbinfo)
```

You said "target server must be a standby" in [1], but I cannot find checks for it.
IIUC, there are two approaches:
 a) check the existence "standby.signal" in the data directory
 b) call an SQL function "pg_is_in_recovery"

04.
```
static char *pg_ctl_path = NULL;
static char *pg_resetwal_path = NULL;
```

I still think they can be combined as "bindir".

05.

```
    /*
     * Write recovery parameters.
...
        WriteRecoveryConfig(conn, opt.subscriber_dir, recoveryconfcontents);
```

WriteRecoveryConfig() writes GUC parameters to postgresql.auto.conf, but not
sure it is good. These settings would remain on new subscriber even after the
pg_createsubscriber. Can we avoid it? I come up with passing these parameters
via pg_ctl -o option, but it requires parsing output from GenerateRecoveryConfig()
(all GUCs must be allign like "-c XXX -c XXX -c XXX...").

06.
```
static LogicalRepInfo *store_pub_sub_info(SimpleStringList dbnames, const char *pub_base_conninfo, const char
*sub_base_conninfo);
...
static void modify_subscriber_sysid(const char *pg_resetwal_path, CreateSubscriberOptions opt);
...
static void wait_for_end_recovery(const char *conninfo, CreateSubscriberOptions opt);
```

Functions arguments should not be struct because they are passing by value.
They should be a pointer. Or, for modify_subscriber_sysid and wait_for_end_recovery,
we can pass a value which would be really used.

07.
```
static char *get_base_conninfo(char *conninfo, char *dbname,
                               const char *noderole);
```

Not sure noderole should be passed here. It is used only for the logging.
Can we output string before calling the function?
(The parameter is not needed anymore if -P is removed)

08.
The terminology is still not consistent. Some functions call the target as standby,
but others call it as subscriber.

09.
v14 does not work if the standby server has already been set recovery_target*
options. PSA the reproducer. I considered two approaches:

 a) raise an ERROR when these parameter were set. check_subscriber() can do it
 b) overwrite these GUCs as empty strings. 

10.
The execution always fails if users execute --dry-run just before. Because
pg_createsubscriber stops the standby anyway. Doing dry run first is quite normal
use-case, so current implementation seems not user-friendly. How should we fix?
Below bullets are my idea:

 a) avoid stopping the standby in case of dry_run: seems possible.
 b) accept even if the standby is stopped: seems possible.
 c) start the standby at the end of run: how arguments like pg_ctl -l should be specified?

My top-up patches fixes some issues.

v15-0001: same as v14-0001
=== experimental patches ===
v15-0002: Use replication connections when we connects to the primary.
          Connections to standby is not changed because the standby/subscriber
          does not require such type of connection, in principle.
          If we can accept connecting to subscriber with replication mode,
          this can be simplified.
v15-0003: Remove -P and use primary_conninfo instead. Same as v13-0004
v15-0004: Check whether the target is really standby. This is done by pg_is_in_recovery()
v15-0005: Avoid stopping/starting standby server in dry_run mode.
          I.e., approach a). in #10 is used.
v15-0006: Overwrite recovery parameters. I.e., aproach b). in #9 is used.

[1]: https://www.postgresql.org/message-id/b315c7da-7ab1-4014-a2a9-8ab6ae26017c%40app.fastmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

On Fri, Feb 2, 2024 at 3:11 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Euler,
>
> Thanks for updating the patch!
>
> >
> I'm still working on the data structures to group options. I don't like the way
> it was grouped in v13-0005. There is too many levels to reach database name.
> The setup_subscriber() function requires the 3 data structures.
> >
>
> Right, your refactoring looks fewer stack. So I pause to revise my refactoring
> patch.
>
> >
> The documentation update is almost there. I will include the modifications in
> the next patch.
> >
>
> OK. I think it should be modified before native speakers will attend to the
> thread.
>
> >
> Regarding v13-0004, it seems a good UI that's why I wrote a comment about it.
> However, it comes with a restriction that requires a similar HBA rule for both
> regular and replication connections. Is it an acceptable restriction? We might
> paint ourselves into the corner. A reasonable proposal is not to remove this
> option. Instead, it should be optional. If it is not provided, primary_conninfo
> is used.
> >
>
> I didn't have such a point of view. However, it is not related whether -P exists
> or not. Even v14-0001 requires primary to accept both normal/replication connections.
> If we want to avoid it, the connection from pg_createsubscriber can be restored
> to replication-connection.
> (I felt we do not have to use replication protocol even if we change the connection mode)
>
> The motivation why -P is not needed is to ensure the consistency of nodes.
> pg_createsubscriber assumes that the -P option can connect to the upstream node,
> but no one checks it. Parsing two connection strings may be a solution but be
> confusing. E.g., what if some options are different?
> I think using a same parameter is a simplest solution.
>
> And below part contains my comments for v14.
>
> 01.
> ```
> char            temp_replslot[NAMEDATALEN] = {0};
> ```
>
> I found that no one refers the name of temporary slot. Can we remove the variable?
>
> 02.
> ```
>         CreateSubscriberOptions opt;
> ...
>         memset(&opt, 0, sizeof(CreateSubscriberOptions));
>
>         /* Default settings */
>         opt.subscriber_dir = NULL;
>         opt.pub_conninfo_str = NULL;
>         opt.sub_conninfo_str = NULL;
>         opt.database_names = (SimpleStringList)
>         {
>                 NULL, NULL
>         };
>         opt.retain = false;
>         opt.recovery_timeout = 0;
> ```
>
> Initialization by `CreateSubscriberOptions opt = {0};` seems enough.
> All values are set to 0x0.
>
> 03.
> ```
> /*
>  * Is the standby server ready for logical replication?
>  */
> static bool
> check_subscriber(LogicalRepInfo *dbinfo)
> ```
>
> You said "target server must be a standby" in [1], but I cannot find checks for it.
> IIUC, there are two approaches:
>  a) check the existence "standby.signal" in the data directory
>  b) call an SQL function "pg_is_in_recovery"
>
> 04.
> ```
> static char *pg_ctl_path = NULL;
> static char *pg_resetwal_path = NULL;
> ```
>
> I still think they can be combined as "bindir".
>
> 05.
>
> ```
>         /*
>          * Write recovery parameters.
> ...
>                 WriteRecoveryConfig(conn, opt.subscriber_dir, recoveryconfcontents);
> ```
>
> WriteRecoveryConfig() writes GUC parameters to postgresql.auto.conf, but not
> sure it is good. These settings would remain on new subscriber even after the
> pg_createsubscriber. Can we avoid it? I come up with passing these parameters
> via pg_ctl -o option, but it requires parsing output from GenerateRecoveryConfig()
> (all GUCs must be allign like "-c XXX -c XXX -c XXX...").
>
> 06.
> ```
> static LogicalRepInfo *store_pub_sub_info(SimpleStringList dbnames, const char *pub_base_conninfo, const char
*sub_base_conninfo);
> ...
> static void modify_subscriber_sysid(const char *pg_resetwal_path, CreateSubscriberOptions opt);
> ...
> static void wait_for_end_recovery(const char *conninfo, CreateSubscriberOptions opt);
> ```
>
> Functions arguments should not be struct because they are passing by value.
> They should be a pointer. Or, for modify_subscriber_sysid and wait_for_end_recovery,
> we can pass a value which would be really used.
>
> 07.
> ```
> static char *get_base_conninfo(char *conninfo, char *dbname,
>                                                            const char *noderole);
> ```
>
> Not sure noderole should be passed here. It is used only for the logging.
> Can we output string before calling the function?
> (The parameter is not needed anymore if -P is removed)
>
> 08.
> The terminology is still not consistent. Some functions call the target as standby,
> but others call it as subscriber.
>
> 09.
> v14 does not work if the standby server has already been set recovery_target*
> options. PSA the reproducer. I considered two approaches:
>
>  a) raise an ERROR when these parameter were set. check_subscriber() can do it
>  b) overwrite these GUCs as empty strings.
>
> 10.
> The execution always fails if users execute --dry-run just before. Because
> pg_createsubscriber stops the standby anyway. Doing dry run first is quite normal
> use-case, so current implementation seems not user-friendly. How should we fix?
> Below bullets are my idea:
>
>  a) avoid stopping the standby in case of dry_run: seems possible.
>  b) accept even if the standby is stopped: seems possible.
>  c) start the standby at the end of run: how arguments like pg_ctl -l should be specified?
>
> My top-up patches fixes some issues.
>
> v15-0001: same as v14-0001
> === experimental patches ===
> v15-0002: Use replication connections when we connects to the primary.
>           Connections to standby is not changed because the standby/subscriber
>           does not require such type of connection, in principle.
>           If we can accept connecting to subscriber with replication mode,
>           this can be simplified.
> v15-0003: Remove -P and use primary_conninfo instead. Same as v13-0004
> v15-0004: Check whether the target is really standby. This is done by pg_is_in_recovery()
> v15-0005: Avoid stopping/starting standby server in dry_run mode.
>           I.e., approach a). in #10 is used.
> v15-0006: Overwrite recovery parameters. I.e., aproach b). in #9 is used.
>
> [1]: https://www.postgresql.org/message-id/b315c7da-7ab1-4014-a2a9-8ab6ae26017c%40app.fastmail.com


While reviewing the v15 patches I discovered that subscription
connection string has added a lot of options which are not required
now:
v15-0001
postgres=# select subname, subconninfo from pg_subscription;
            subname            |               subconninfo
-------------------------------+------------------------------------------
pg_createsubscriber_5_1867633 | host=localhost port=5432 dbname=postgres
(1 row)



v15-0001+0002+0003
postgres=# select subname, subconninfo from pg_subscription;
            subname            |

      subconninfo



-------------------------------+------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------
------------------------------
pg_createsubscriber_5_1895366 | user=shubham
passfile='/home/shubham/.pgpass' channel_binding=prefer ho
st=127.0.0.1 port=5432 sslmode=prefer sslcompression=0
sslcertmode=allow sslsni=1 ssl_min_protocol_versi
on=TLSv1.2 gssencmode=disable krbsrvname=postgres gssdelegation=0
target_session_attrs=any load_balance_
hosts=disable dbname=postgres
(1 row)


Here, we can see that channel_binding, sslmode, sslcertmode, sslsni,
gssencmode, krbsrvname, etc are getting included. This does not look
intentional, we should keep the subscription connection same as in
v15-0001.

Thanks and Regards,
Shubham Khanna.


Thanks and Regards,
Shubham Khanna.

Dear Euler,

> 03.
> ```
> /*
>  * Is the standby server ready for logical replication?
>  */
> static bool
> check_subscriber(LogicalRepInfo *dbinfo)
> ```
> 
> You said "target server must be a standby" in [1], but I cannot find checks for it.
> IIUC, there are two approaches:
>  a) check the existence "standby.signal" in the data directory
>  b) call an SQL function "pg_is_in_recovery"

I found that current code (HEAD+v14-0001) leads stuck or timeout because the recovery
would be never finished. In attached script emulates the case standby.signal file is missing.
This script always fails with below output:

```
...
pg_createsubscriber: waiting the postmaster to reach the consistent state
pg_createsubscriber: postmaster was stopped
pg_createsubscriber: error: recovery timed out
...
```

I also attached the server log during pg_createsubscriber, and we can see that
walreceiver exits before receiving all the required changes. I cannot find a path
yet, but the inconsistency lead the situation which walreceiver exists but startup
remains. This also said that recovery status must be checked in check_subscriber().

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear Shubham,

Thanks for testing our codes!

> While reviewing the v15 patches I discovered that subscription
> connection string has added a lot of options which are not required
> now:
> v15-0001
> postgres=# select subname, subconninfo from pg_subscription;
>             subname            |               subconninfo
> -------------------------------+------------------------------------------
> pg_createsubscriber_5_1867633 | host=localhost port=5432 dbname=postgres
> (1 row)
> 
> 
> 
> v15-0001+0002+0003
> postgres=# select subname, subconninfo from pg_subscription;
>             subname            |
> 
>       subconninfo
> 
> 
> 
> -------------------------------+--------------------------------------------------
> ----------------------
> ----------------------------------------------------------------------------------
> ----------------------
> ----------------------------------------------------------------------------------
> ----------------------
> ------------------------------
> pg_createsubscriber_5_1895366 | user=shubham
> passfile='/home/shubham/.pgpass' channel_binding=prefer ho
> st=127.0.0.1 port=5432 sslmode=prefer sslcompression=0
> sslcertmode=allow sslsni=1 ssl_min_protocol_versi
> on=TLSv1.2 gssencmode=disable krbsrvname=postgres gssdelegation=0
> target_session_attrs=any load_balance_
> hosts=disable dbname=postgres
> (1 row)
> 
> 
> Here, we can see that channel_binding, sslmode, sslcertmode, sslsni,
> gssencmode, krbsrvname, etc are getting included. This does not look
> intentional, we should keep the subscription connection same as in
> v15-0001.

You should attach the script the reproducer. I suspected you used pg_basebackup
-R command for setting up the standby. In this case, it is intentional.
These settings are not caused by the pg_createsubscriber, done by pg_basebackup.
If you set primary_conninfo manually like attached, these settings would not appear.

As the first place, listed options (E.g., passfile, channel_binding, sslmode,
sslcompression, sslcertmode, etc...) were set when you connect to the database
via libpq functions. PQconninfoOptions in fe-connect.c lists parameters and
their default value.

v15-0003 reuses the primary_conninfo for subconninfo attribute. primary_conninfo
is set by pg_basebackup specified with '-R' option.
The content is built in GenerateRecoveryConfig(), which bypass parameters from
PQconninfo(). This function returns all the libpq connection parameters even if
it is set as default. So primary_conninfo looks longer.

I don't think this works wrongly. Users still can set an arbitrary connection
string as primary_conninfo. You just use longer string unintentionally.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Hi,

> My top-up patches fixes some issues.
>
> v15-0001: same as v14-0001
> === experimental patches ===
> v15-0002: Use replication connections when we connects to the primary.
>           Connections to standby is not changed because the standby/subscriber
>           does not require such type of connection, in principle.
>           If we can accept connecting to subscriber with replication mode,
>           this can be simplified.
> v15-0003: Remove -P and use primary_conninfo instead. Same as v13-0004
> v15-0004: Check whether the target is really standby. This is done by pg_is_in_recovery()
> v15-0005: Avoid stopping/starting standby server in dry_run mode.
>           I.e., approach a). in #10 is used.
> v15-0006: Overwrite recovery parameters. I.e., aproach b). in #9 is used.
>
> [1]: https://www.postgresql.org/message-id/b315c7da-7ab1-4014-a2a9-8ab6ae26017c%40app.fastmail.com
>

I have created a topup patch 0007 on top of v15-0006.

I revived the patch which removes -S option and adds some options
instead. The patch add option for --port, --username and --socketdir.
This patch also ensures that anyone cannot connect to the standby
during the pg_createsubscriber, by setting listen_addresses,
unix_socket_permissions, and unix_socket_directories.

Thanks and Regards,
Shlok Kyal

Thanks for updating the patch!

>

I'm still working on the data structures to group options. I don't like the way

it was grouped in v13-0005. There is too many levels to reach database name.

The setup_subscriber() function requires the 3 data structures.

>

Right, your refactoring looks fewer stack. So I pause to revise my refactoring

patch.

>

The documentation update is almost there. I will include the modifications in

the next patch.

>

OK. I think it should be modified before native speakers will attend to the

thread.

>

Regarding v13-0004, it seems a good UI that's why I wrote a comment about it.

However, it comes with a restriction that requires a similar HBA rule for both

regular and replication connections. Is it an acceptable restriction? We might

paint ourselves into the corner. A reasonable proposal is not to remove this

option. Instead, it should be optional. If it is not provided, primary_conninfo

is used.

>

I didn't have such a point of view. However, it is not related whether -P exists

or not. Even v14-0001 requires primary to accept both normal/replication connections.

If we want to avoid it, the connection from pg_createsubscriber can be restored

to replication-connection.

(I felt we do not have to use replication protocol even if we change the connection mode)

The motivation why -P is not needed is to ensure the consistency of nodes.

pg_createsubscriber assumes that the -P option can connect to the upstream node,

but no one checks it. Parsing two connection strings may be a solution but be

confusing. E.g., what if some options are different?

I think using a same parameter is a simplest solution.

I found that no one refers the name of temporary slot. Can we remove the variable?

Initialization by `CreateSubscriberOptions opt = {0};` seems enough.

All values are set to 0x0.

You said "target server must be a standby" in [1], but I cannot find checks for it.

IIUC, there are two approaches:

a) check the existence "standby.signal" in the data directory

b) call an SQL function "pg_is_in_recovery"

I still think they can be combined as "bindir".

WriteRecoveryConfig() writes GUC parameters to postgresql.auto.conf, but not

sure it is good. These settings would remain on new subscriber even after the

pg_createsubscriber. Can we avoid it? I come up with passing these parameters

via pg_ctl -o option, but it requires parsing output from GenerateRecoveryConfig()

(all GUCs must be allign like "-c XXX -c XXX -c XXX...").

Functions arguments should not be struct because they are passing by value.

They should be a pointer. Or, for modify_subscriber_sysid and wait_for_end_recovery,

we can pass a value which would be really used.

07.

```

static char *get_base_conninfo(char *conninfo, char *dbname,

   const char *noderole);

```

Not sure noderole should be passed here. It is used only for the logging.

Can we output string before calling the function?

(The parameter is not needed anymore if -P is removed)

08.

The terminology is still not consistent. Some functions call the target as standby,

but others call it as subscriber.

09.

v14 does not work if the standby server has already been set recovery_target*

options. PSA the reproducer. I considered two approaches:

a) raise an ERROR when these parameter were set. check_subscriber() can do it

b) overwrite these GUCs as empty strings. 

10.

The execution always fails if users execute --dry-run just before. Because

pg_createsubscriber stops the standby anyway. Doing dry run first is quite normal

use-case, so current implementation seems not user-friendly. How should we fix?

Below bullets are my idea:

a) avoid stopping the standby in case of dry_run: seems possible.

b) accept even if the standby is stopped: seems possible.

c) start the standby at the end of run: how arguments like pg_ctl -l should be specified?

Dear Shlok,

Thanks for updating the patch!

> I have created a topup patch 0007 on top of v15-0006.
> 
> I revived the patch which removes -S option and adds some options
> instead. The patch add option for --port, --username and --socketdir.
> This patch also ensures that anyone cannot connect to the standby
> during the pg_createsubscriber, by setting listen_addresses,
> unix_socket_permissions, and unix_socket_directories.

IIUC, there are two reasons why removing -S may be good:

* force users to specify a local-connection, and
* avoid connection establishment on standby during the pg_createsubscriber.

First bullet is still valid, but we should describe that like pg_upgrade: 

>
pg_upgrade will connect to the old and new servers several times, so you might
want to set authentication to peer in pg_hba.conf or use a ~/.pgpass file
(see Section 33.16).
>

Regarding the second bullet, this patch cannot ensure it. pg_createsubscriber
just accepts port number which has been already accepted by the standby, it does
not change the port number. So any local applications on the standby server can
connect to the server and may change primary_conninfo, primary_slot_name, data, etc.
So...what should be? How do other think?

Beside, 0007 does not follow the recent changes on 0001. E.g., options should be
record in CreateSubscriberOptions. Also, should we check the privilege of socket
directory?

[1]:
https://www.postgresql.org/message-id/TY3PR01MB988902B992A4F2E99E1385EDF56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear Euler,

Sorry for posting e-mail each other. I will read your patch
but I want to reply one of yours.

>
I started reviewing v16-0007 but didn't finish yet. The general idea is ok.
However, I'm still worried about preventing some use cases if it provides only
the local connection option. What if you want to keep monitoring this instance
while the transformation is happening? Let's say it has a backlog that will
take some time to apply. Unless, you have a local agent, you have no data about
this server until pg_createsubscriber terminates. Even the local agent might
not connect to the server unless you use the current port.
>

(IIUC, 0007 could not overwrite a port number - refactoring is needed)

Ah, actually I did not have such a point of view. Assuming that changed port number
can avoid connection establishments, there are four options:
a) Does not overwrite port and listen_addresses. This allows us to monitor by
   external agents, but someone can modify GUCs and data during operations.
b) Overwrite port but do not do listen_addresses. Not sure it is useful... 
c) Overwrite listen_addresses but do not do port. This allows us to monitor by
   local agents, and we can partially protect the database. But there is still a 
   room.
d) Overwrite both port and listen_addresses. This can protect databases perfectly
but no one can monitor.

Hmm, which one should be chosen? I prefer c) or d).
Do you know how pglogical_create_subscriber does?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

Dear Euler,

I have not finished reviewing, but I can reply to you first.

>
I didn't complete this task yet so I didn't include it in this patch.
>

Just to confirm - No need to forcibly include my refactoring patch: you can
modify based on your idea.

>
That's correct. We made a decision to use non physical replication connections
(besides the one used to connect primary <-> standby). Hence, my concern about
a HBA rule falls apart. I'm not convinced that using a modified
primary_conninfo is the only/right answer to fill the subscription connection
string. Physical vs logical replication has different requirements when we talk
about users. The physical replication requires only the REPLICATION privilege.
On the other hand, to create a subscription you must have the privileges of
pg_create_subscription role and also CREATE privilege on the specified
database. Unless, you are always recommending the superuser for this tool, I'm
afraid there will be cases that reusing primary_conninfo will be an issue. The
more I think about this UI, the more I think that, if it is not hundreds of
lines of code, it uses the primary_conninfo the -P is not specified.
>

Valid point. It is one of the best practice that users set minimal privileges
for each accounts. However...

>
Ugh. An error will occur the first time (get_primary_sysid) it tries to connect
to primary.
>

I'm not sure it is correct. I think there are several patterns.

a) -P option specified a ghost server, i.e., pg_createsubscriber cannot connect to
   anything. In this case, get_primary_sysid() would be failed because
   connect_database() would be failed.

b) -P option specified an existing server, but it is not my upstream.
   get_primary_sysid() would be suceeded.
   In this case, there are two furher branches:
   
   b-1) nodes have different system_identifier. pg_createsubscriber would raise
        an ERROR and stop.
   b-2) nodes have the same system_identifier (e.g., nodes were generated by the
        same master). In this case, current checkings would be passed but
        pg_createsubscriber would stuck or reach timeout. PSA the reproducer.

Can pg_createsubscriber check the relation two nodes have been connected by
replication protocol? Or, can we assume that all the node surely have different
system_identifier?

>
It is. However, I keep the assignments for 2 reasons: (a) there might be
parameters whose default value is not zero, (b) the standard does not say that
a null pointer must be represented by zero and (c) there is no harm in being
paranoid during initial assignment.
>

Hmn, so, no need to use `= {0};`, but up to you. Also, according to C99 standard[1],
NULL seemed to be defined as 0x0:
```
An integer constant expression with the value 0, or such an expression cast to type
void *, is called a null pointer constant.
If a null pointer constant is converted to a
pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal
to a pointer to any object or function.
```

>
> WriteRecoveryConfig() writes GUC parameters to postgresql.auto.conf, but not
> sure it is good. These settings would remain on new subscriber even after the
> pg_createsubscriber. Can we avoid it? I come up with passing these parameters
> via pg_ctl -o option, but it requires parsing output from GenerateRecoveryConfig()
> (all GUCs must be allign like "-c XXX -c XXX -c XXX...").

I applied a modified version of v16-0006.
>

Sorry for confusing, it is not a solution. This approach writes parameter
settings to postgresql.auto.conf, and they are never removed. Overwritten
settings would remain.

>
I don't understand why v16-0002 is required. In a previous version, this patch
was using connections in logical replication mode. After some discussion we
decided to change it to regular connections and use SQL functions (instead of
replication commands). Is it a requirement for v16-0003?
>

No, it is not required by 0003. I forgot the decision that we force DBAs to open
normal connection establishments for pg_createsubscriber. Please ignore...

[1]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf


Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

Ah, actually I did not have such a point of view. Assuming that changed port number

can avoid connection establishments, there are four options:

a) Does not overwrite port and listen_addresses. This allows us to monitor by

   external agents, but someone can modify GUCs and data during operations.

b) Overwrite port but do not do listen_addresses. Not sure it is useful... 

c) Overwrite listen_addresses but do not do port. This allows us to monitor by

   local agents, and we can partially protect the database. But there is still a 

   room.

d) Overwrite both port and listen_addresses. This can protect databases perfectly

but no one can monitor.

Hmm, which one should be chosen? I prefer c) or d).

Do you know how pglogical_create_subscriber does?

Dear Euler,

>
Remember the target server was a standby (read only access). I don't expect an
application trying to modify it; unless it is a buggy application.
>

What if the client modifies the data just after the promotion?
Naively considered, all the changes can be accepted, but are there any issues?

>
Regarding
GUCs, almost all of them is PGC_POSTMASTER (so it cannot be modified unless the
server is restarted). The ones that are not PGC_POSTMASTER, does not affect the
pg_createsubscriber execution [1].
>

IIUC,  primary_conninfo and primary_slot_name is PGC_SIGHUP.

>
I'm just pointing out that this case is a different from pg_upgrade (from which
this idea was taken). I'm not saying that's a bad idea. I'm just arguing that
you might be preventing some access read only access (monitoring) when it is
perfectly fine to connect to the database and execute queries. As I said
before, the current UI allows anyone to setup the standby to accept only local
connections. Of course, it is an extra step but it is possible. However, once
you apply v16-0007, there is no option but use only local connection during the
transformation. Is it an acceptable limitation?
>

My remained concern is written above. If they do not problematic we may not have
to restrict them for now. At that time, changes 

1) overwriting a port number,
2) setting listen_addresses = ''

are not needed, right? IIUC inconsistency of -P may be still problematic.

>
pglogical_create_subscriber does nothing [2][3].
>

Oh, thanks.
Just to confirm - pglogical set shared_preload_libraries to '', should we follow or not?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

Dear Euler,

Here are my minor comments for 17.

01.
```
/* Options */
static const char *progname;

static char *primary_slot_name = NULL;
static bool dry_run = false;

static bool success = false;

static LogicalRepInfo *dbinfo;
static int    num_dbs = 0;
```

The comment seems out-of-date. There is only one option.

02. check_subscriber and check_publisher

Missing pg_catalog prefix in some lines.

03. get_base_conninfo

I think dbname would not be set. IIUC, dbname should be a pointer of the pointer.


04.

I check the coverage and found two functions have been never called:
 - drop_subscription
 - drop_replication_slot

Also, some cases were not tested. Below bullet showed notable ones for me.
(Some of them would not be needed based on discussions)

* -r is specified
* -t is specified
* -P option contains dbname
* -d is not specified
* GUC settings are wrong
* primary_slot_name is specified on the standby
* standby server is not working

In feature level, we may able to check the server log is surely removed in case
of success.

So, which tests should be added? drop_subscription() is called only when the
cleanup phase, so it may be difficult to test. According to others, it seems that
-r and -t are not tested. GUC-settings have many test cases so not sure they
should be. Based on this, others can be tested.

PSA my top-up patch set.

V18-0001: same as your patch, v17-0001.
V18-0002: modify the alignment of codes.
V18-0003: change an argument of get_base_conninfo. Per comment 3.
=== experimental patches ===
V18-0004: Add testcases per comment 4.
V18-0005: Remove -P option. I'm not sure it should be needed, but I made just in case.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

On Wed, 7 Feb 2024 at 10:24, Euler Taveira <euler@eulerto.com> wrote:
>
> On Fri, Feb 2, 2024, at 6:41 AM, Hayato Kuroda (Fujitsu) wrote:
>
> Thanks for updating the patch!

Thanks for the updated patch, few comments:
Few comments:
1) Cleanup function handler flag should be reset, i.e.
dbinfo->made_replslot = false; should be there else there will be an
error during  drop replication slot cleanup in error flow:
+static void
+drop_replication_slot(PGconn *conn, LogicalRepInfo *dbinfo, const
char *slot_name)
+{
+       PQExpBuffer str = createPQExpBuffer();
+       PGresult   *res;
+
+       Assert(conn != NULL);
+
+       pg_log_info("dropping the replication slot \"%s\" on database
\"%s\"", slot_name, dbinfo->dbname);
+
+       appendPQExpBuffer(str, "SELECT
pg_drop_replication_slot('%s')", slot_name);
+
+       pg_log_debug("command is: %s", str->data);

2) Cleanup function handler flag should be reset, i.e.
dbinfo->made_publication = false; should be there else there will be
an error during drop publication cleanup in error flow:
+/*
+ * Remove publication if it couldn't finish all steps.
+ */
+static void
+drop_publication(PGconn *conn, LogicalRepInfo *dbinfo)
+{
+       PQExpBuffer str = createPQExpBuffer();
+       PGresult   *res;
+
+       Assert(conn != NULL);
+
+       pg_log_info("dropping publication \"%s\" on database \"%s\"",
dbinfo->pubname, dbinfo->dbname);
+
+       appendPQExpBuffer(str, "DROP PUBLICATION %s", dbinfo->pubname);
+
+       pg_log_debug("command is: %s", str->data);

3) Cleanup function handler flag should be reset, i.e.
dbinfo->made_subscription = false; should be there else there will be
an error during drop publication cleanup in error flow:
+/*
+ * Remove subscription if it couldn't finish all steps.
+ */
+static void
+drop_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
+{
+       PQExpBuffer str = createPQExpBuffer();
+       PGresult   *res;
+
+       Assert(conn != NULL);
+
+       pg_log_info("dropping subscription \"%s\" on database \"%s\"",
dbinfo->subname, dbinfo->dbname);
+
+       appendPQExpBuffer(str, "DROP SUBSCRIPTION %s", dbinfo->subname);
+
+       pg_log_debug("command is: %s", str->data);

4) I was not sure if drop_publication is required here, as we will not
create any publication in subscriber node:
+               if (dbinfo[i].made_subscription)
+               {
+                       conn = connect_database(dbinfo[i].subconninfo);
+                       if (conn != NULL)
+                       {
+                               drop_subscription(conn, &dbinfo[i]);
+                               if (dbinfo[i].made_publication)
+                                       drop_publication(conn, &dbinfo[i]);
+                               disconnect_database(conn);
+                       }
+               }

5) The connection should be disconnected in case of error case:
+       /* secure search_path */
+       res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
+       if (PQresultStatus(res) != PGRES_TUPLES_OK)
+       {
+               pg_log_error("could not clear search_path: %s",
PQresultErrorMessage(res));
+               return NULL;
+       }
+       PQclear(res);

6) There should be a line break before postgres_fe inclusion, to keep
it consistent:
+ *-------------------------------------------------------------------------
+ */
+#include "postgres_fe.h"
+
+#include <signal.h>

7) These includes are not required:
7.a) #include <signal.h>
7.b) #include <sys/stat.h>
7.c) #include <sys/wait.h>
7.d) #include "access/xlogdefs.h"
7.e) #include "catalog/pg_control.h"
7.f) #include "common/file_utils.h"
7.g) #include "utils/pidfile.h"

+ *             src/bin/pg_basebackup/pg_createsubscriber.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres_fe.h"
+
+#include <signal.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+#include <time.h>
+
+#include "access/xlogdefs.h"
+#include "catalog/pg_authid_d.h"
+#include "catalog/pg_control.h"
+#include "common/connect.h"
+#include "common/controldata_utils.h"
+#include "common/file_perm.h"
+#include "common/file_utils.h"
+#include "common/logging.h"
+#include "common/restricted_token.h"
+#include "fe_utils/recovery_gen.h"
+#include "fe_utils/simple_list.h"
+#include "getopt_long.h"
+#include "utils/pidfile.h"

8) POSTMASTER_STANDBY and POSTMASTER_FAILED are not being used, is it
required or kept for future purpose:
+enum WaitPMResult
+{
+       POSTMASTER_READY,
+       POSTMASTER_STANDBY,
+       POSTMASTER_STILL_STARTING,
+       POSTMASTER_FAILED
+};

9) pg_createsubscriber should be kept after pg_basebackup to maintain
the consistent order:
diff --git a/src/bin/pg_basebackup/.gitignore b/src/bin/pg_basebackup/.gitignore
index 26048bdbd8..b3a6f5a2fe 100644
--- a/src/bin/pg_basebackup/.gitignore
+++ b/src/bin/pg_basebackup/.gitignore
@@ -1,5 +1,6 @@
 /pg_basebackup
 /pg_receivewal
 /pg_recvlogical
+/pg_createsubscriber

10) dry-run help message is not very clear, how about something
similar to pg_upgrade's message like "check clusters only, don't
change any data":
+       printf(_(" -d, --database=DBNAME               database to
create a subscription\n"));
+       printf(_(" -n, --dry-run                       stop before
modifying anything\n"));
+       printf(_(" -t, --recovery-timeout=SECS         seconds to wait
for recovery to end\n"));
+       printf(_(" -r, --retain                        retain log file
after success\n"));
+       printf(_(" -v, --verbose                       output verbose
messages\n"));
+       printf(_(" -V, --version                       output version
information, then exit\n"));

Regards,
Vignesh

On Wed, Feb 7, 2024 at 10:24 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Fri, Feb 2, 2024, at 6:41 AM, Hayato Kuroda (Fujitsu) wrote:
>
> Thanks for updating the patch!
>
>
> Thanks for taking a look.
>
> >
> I'm still working on the data structures to group options. I don't like the way
> it was grouped in v13-0005. There is too many levels to reach database name.
> The setup_subscriber() function requires the 3 data structures.
> >
>
> Right, your refactoring looks fewer stack. So I pause to revise my refactoring
> patch.
>
>
> I didn't complete this task yet so I didn't include it in this patch.
>
> >
> The documentation update is almost there. I will include the modifications in
> the next patch.
> >
>
> OK. I think it should be modified before native speakers will attend to the
> thread.
>
>
> Same for this one.
>
> >
> Regarding v13-0004, it seems a good UI that's why I wrote a comment about it.
> However, it comes with a restriction that requires a similar HBA rule for both
> regular and replication connections. Is it an acceptable restriction? We might
> paint ourselves into the corner. A reasonable proposal is not to remove this
> option. Instead, it should be optional. If it is not provided, primary_conninfo
> is used.
> >
>
> I didn't have such a point of view. However, it is not related whether -P exists
> or not. Even v14-0001 requires primary to accept both normal/replication connections.
> If we want to avoid it, the connection from pg_createsubscriber can be restored
> to replication-connection.
> (I felt we do not have to use replication protocol even if we change the connection mode)
>
>
> That's correct. We made a decision to use non physical replication connections
> (besides the one used to connect primary <-> standby). Hence, my concern about
> a HBA rule falls apart. I'm not convinced that using a modified
> primary_conninfo is the only/right answer to fill the subscription connection
> string. Physical vs logical replication has different requirements when we talk
> about users. The physical replication requires only the REPLICATION privilege.
> On the other hand, to create a subscription you must have the privileges of
> pg_create_subscription role and also CREATE privilege on the specified
> database. Unless, you are always recommending the superuser for this tool, I'm
> afraid there will be cases that reusing primary_conninfo will be an issue. The
> more I think about this UI, the more I think that, if it is not hundreds of
> lines of code, it uses the primary_conninfo the -P is not specified.
>
> The motivation why -P is not needed is to ensure the consistency of nodes.
> pg_createsubscriber assumes that the -P option can connect to the upstream node,
> but no one checks it. Parsing two connection strings may be a solution but be
> confusing. E.g., what if some options are different?
> I think using a same parameter is a simplest solution.
>
>
> Ugh. An error will occur the first time (get_primary_sysid) it tries to connect
> to primary.
>
> I found that no one refers the name of temporary slot. Can we remove the variable?
>
>
> It is gone. I did a refactor in the create_logical_replication_slot function.
> Slot name is assigned internally (no need for slot_name or temp_replslot) and
> temporary parameter is included.
>
> Initialization by `CreateSubscriberOptions opt = {0};` seems enough.
> All values are set to 0x0.
>
>
> It is. However, I keep the assignments for 2 reasons: (a) there might be
> parameters whose default value is not zero, (b) the standard does not say that
> a null pointer must be represented by zero and (c) there is no harm in being
> paranoid during initial assignment.
>
> You said "target server must be a standby" in [1], but I cannot find checks for it.
> IIUC, there are two approaches:
> a) check the existence "standby.signal" in the data directory
> b) call an SQL function "pg_is_in_recovery"
>
>
> I applied v16-0004 that implements option (b).
>
> I still think they can be combined as "bindir".
>
>
> I applied a patch that has a single variable for BINDIR.
>
> WriteRecoveryConfig() writes GUC parameters to postgresql.auto.conf, but not
> sure it is good. These settings would remain on new subscriber even after the
> pg_createsubscriber. Can we avoid it? I come up with passing these parameters
> via pg_ctl -o option, but it requires parsing output from GenerateRecoveryConfig()
> (all GUCs must be allign like "-c XXX -c XXX -c XXX...").
>
>
> I applied a modified version of v16-0006.
>
> Functions arguments should not be struct because they are passing by value.
> They should be a pointer. Or, for modify_subscriber_sysid and wait_for_end_recovery,
> we can pass a value which would be really used.
>
>
> Done.
>
> 07.
> ```
> static char *get_base_conninfo(char *conninfo, char *dbname,
>    const char *noderole);
> ```
>
> Not sure noderole should be passed here. It is used only for the logging.
> Can we output string before calling the function?
> (The parameter is not needed anymore if -P is removed)
>
>
> Done.
>
> 08.
> The terminology is still not consistent. Some functions call the target as standby,
> but others call it as subscriber.
>
>
> The terminology should reflect the actual server role. I'm calling it "standby"
> if it is a physical replica and "subscriber" if it is a logical replica. Maybe
> "standby" isn't clear enough.
>
> 09.
> v14 does not work if the standby server has already been set recovery_target*
> options. PSA the reproducer. I considered two approaches:
>
> a) raise an ERROR when these parameter were set. check_subscriber() can do it
> b) overwrite these GUCs as empty strings.
>
>
> I prefer (b) that's exactly what you provided in v16-0006.
>
> 10.
> The execution always fails if users execute --dry-run just before. Because
> pg_createsubscriber stops the standby anyway. Doing dry run first is quite normal
> use-case, so current implementation seems not user-friendly. How should we fix?
> Below bullets are my idea:
>
> a) avoid stopping the standby in case of dry_run: seems possible.
> b) accept even if the standby is stopped: seems possible.
> c) start the standby at the end of run: how arguments like pg_ctl -l should be specified?
>
>
> I prefer (a). I applied a slightly modified version of v16-0005.
>
> This new patch contains the following changes:
>
> * check whether the target is really a standby server (0004)
> * refactor: pg_create_logical_replication_slot function
> * use a single variable for pg_ctl and pg_resetwal directory
> * avoid recovery errors applying default settings for some GUCs (0006)
> * don't stop/start the standby in dry run mode (0005)
> * miscellaneous fixes
>
> I don't understand why v16-0002 is required. In a previous version, this patch
> was using connections in logical replication mode. After some discussion we
> decided to change it to regular connections and use SQL functions (instead of
> replication commands). Is it a requirement for v16-0003?
>
> I started reviewing v16-0007 but didn't finish yet. The general idea is ok.
> However, I'm still worried about preventing some use cases if it provides only
> the local connection option. What if you want to keep monitoring this instance
> while the transformation is happening? Let's say it has a backlog that will
> take some time to apply. Unless, you have a local agent, you have no data about
> this server until pg_createsubscriber terminates. Even the local agent might
> not connect to the server unless you use the current port.

I tried verifying few scenarios by using 5 databases and came across
the following errors:

./pg_createsubscriber -D ../new_standby -P "host=localhost port=5432
dbname=postgres" -S "host=localhost port=9000 dbname=postgres"  -d db1
-d db2 -d db3 -d db4 -d db5

pg_createsubscriber: error: publisher requires 6 wal sender
processes, but only 5 remain
pg_createsubscriber: hint: Consider increasing max_wal_senders to at least 7.

It is successful only with 7 wal senders, so we can change error
messages accordingly.


pg_createsubscriber: error: publisher requires 6 replication slots,
but only 5 remain
pg_createsubscriber: hint: Consider increasing max_replication_slots
to at least 7.

It is successful only with 7 replication slots, so we can change error
messages accordingly.

Thanks and Regards,
Shubham Khanna,

Dear Euler,

Further comments for v17.

01.
This program assumes that the target server has same major version with this.
Because the target server would be restarted by same version's pg_ctl command.
I felt it should be ensured by reading the PG_VERSION.

02.
pg_upgrade checked the version of using executables, like pg_ctl, postgres, and
pg_resetwal. I felt it should be as well.

03. get_bin_directory
```
    if (find_my_exec(path, full_path) < 0)
    {
        pg_log_error("The program \"%s\" is needed by %s but was not found in the\n"
                     "same directory as \"%s\".\n",
                     "pg_ctl", progname, full_path);
```

s/"pg_ctl"/progname

04.
Missing canonicalize_path()?

05.
Assuming that the target server is a cascade standby, i.e., it has a role as
another primary. In this case, I thought the child node would not work. Because
pg_createsubcriber runs pg_resetwal and all WAL files would be discarded at that
time. I have not tested, but should the program detect it and exit earlier?

06.
wait_for_end_recovery() waits forever even if the standby has been disconnected
from the primary, right? should we check the status of the replication via
pg_stat_wal_receiver?

07.
The cleanup function has couple of bugs.

* If subscriptions have been created on the database, the function also tries to
  drop a publication. But it leads an ERROR because it has been already dropped.
  See setup_subscriber().
* If the subscription has been created, drop_replication_slot() leads an ERROR.
  Because the subscriber tried to drop the subscription while executing DROP SUBSCRIPTION.

08.
I found that all messages (ERROR, WARNING, INFO, etc...) would output to stderr,
but I felt it should be on stdout. Is there a reason? pg_dump outputs messages to
stderr, but the motivation might be to avoid confusion with dumps.

09.
I'm not sure the cleanup for subscriber is really needed. Assuming that there
are two databases, e.g., pg1 pg2 , and we fail to create a subscription on pg2.
This can happen when the subscription which has the same name has been already
created on the primary server.
In this case a subscirption pn pg1 would be removed. But what is a next step?
Since a timelineID on the standby server is larger than the primary (note that
the standby has been promoted once), we cannot resume the physical replication
as-is. IIUC the easiest method to retry is removing a cluster once and restarting
from pg_basebackup. If so, no need to cleanup the standby because it is corrupted.
We just say "Please remove the cluster and recreate again".

Here is a reproducer.

1. apply the txt patch atop 0001 patch.
2. run test_corruption.sh.
3. when you find a below output [1], connect to a testdb from another terminal and
   run CREATE SUBSCRITPION for the same subscription on the primary
4. Finally, pg_createsubscriber would fail the creation.

I also attached server logs of both nodes and the output.
Note again that this is a real issue. I used a tricky way for surely overlapping name,
but this can happen randomly.

10.
While investigating #09, I found that we cannot report properly a reason why the
subscription cannot be created. The output said:

```
pg_createsubscriber: error: could not create subscription "pg_createsubscriber_16389_3884" on database "testdb": out of
memory
```

But the standby serverlog said:

```
ERROR:  subscription "pg_createsubscriber_16389_3884" already exists
STATEMENT:  CREATE SUBSCRIPTION pg_createsubscriber_16389_3884 CONNECTION 'user=postgres port=5431 dbname=testdb'
PUBLICATIONpg_createsubscriber_16389 WITH (create_slot = false, copy_data = false, enabled = false)
 
```

[1]
```
pg_createsubscriber: creating the replication slot "pg_createsubscriber_16389_3884" on database "testdb"
pg_createsubscriber: XXX: sleep 20s
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

Dear hackers,

Since the original author seems bit busy, I updated the patch set.

> 
> 01.
> ```
> /* Options */
> static const char *progname;
> 
> static char *primary_slot_name = NULL;
> static bool dry_run = false;
> 
> static bool success = false;
> 
> static LogicalRepInfo *dbinfo;
> static int    num_dbs = 0;
> ```
> 
> The comment seems out-of-date. There is only one option.

Changed the comment to /* Global variables */.

> 
> 02. check_subscriber and check_publisher
> 
> Missing pg_catalog prefix in some lines.

This has been already addressed in v18.

> 03. get_base_conninfo
> 
> I think dbname would not be set. IIUC, dbname should be a pointer of the pointer.

This has been already addressed in v18.

> 04.
> 
> I check the coverage and found two functions have been never called:
>  - drop_subscription
>  - drop_replication_slot
> 
> Also, some cases were not tested. Below bullet showed notable ones for me.
> (Some of them would not be needed based on discussions)
> 
> * -r is specified
> * -t is specified
> * -P option contains dbname
> * -d is not specified
> * GUC settings are wrong
> * primary_slot_name is specified on the standby
> * standby server is not working
> 
> In feature level, we may able to check the server log is surely removed in case
> of success.
> 
> So, which tests should be added? drop_subscription() is called only when the
> cleanup phase, so it may be difficult to test. According to others, it seems that
> -r and -t are not tested. GUC-settings have many test cases so not sure they
> should be. Based on this, others can be tested.

This has been already addressed in v18.

PSA my top-up patch set.

V19-0001: same as Euler's patch, v17-0001.
V19-0002: Update docs per recent changes. Also, some adjustments were done.
V19-0003: Modify the alignment of codes. Mostly same as v18-0002.
V19-0004: Change an argument of get_base_conninfo. Same as v18-0003.
=== experimental patches ===
V19-0005: Add testcases. Same as v18-0004.
V19-0006: Update a comment above global variables.
V19-0007: Address comments from Vignesh.
V19-0008: Fix error message in get_bin_directory().
V19-0009: Remove -P option. Same as v18-0005. 

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

Dear Vignesh,

Since the original author seems bit busy, I updated the patch set.

> 1) Cleanup function handler flag should be reset, i.e.
> dbinfo->made_replslot = false; should be there else there will be an
> error during  drop replication slot cleanup in error flow:
> +static void
> +drop_replication_slot(PGconn *conn, LogicalRepInfo *dbinfo, const
> char *slot_name)
> +{
> +       PQExpBuffer str = createPQExpBuffer();
> +       PGresult   *res;
> +
> +       Assert(conn != NULL);
> +
> +       pg_log_info("dropping the replication slot \"%s\" on database
> \"%s\"", slot_name, dbinfo->dbname);
> +
> +       appendPQExpBuffer(str, "SELECT
> pg_drop_replication_slot('%s')", slot_name);
> +
> +       pg_log_debug("command is: %s", str->data);

Fixed.

> 2) Cleanup function handler flag should be reset, i.e.
> dbinfo->made_publication = false; should be there else there will be
> an error during drop publication cleanup in error flow:
> +/*
> + * Remove publication if it couldn't finish all steps.
> + */
> +static void
> +drop_publication(PGconn *conn, LogicalRepInfo *dbinfo)
> +{
> +       PQExpBuffer str = createPQExpBuffer();
> +       PGresult   *res;
> +
> +       Assert(conn != NULL);
> +
> +       pg_log_info("dropping publication \"%s\" on database \"%s\"",
> dbinfo->pubname, dbinfo->dbname);
> +
> +       appendPQExpBuffer(str, "DROP PUBLICATION %s", dbinfo->pubname);
> +
> +       pg_log_debug("command is: %s", str->data);
> 
> 3) Cleanup function handler flag should be reset, i.e.
> dbinfo->made_subscription = false; should be there else there will be
> an error during drop publication cleanup in error flow:
> +/*
> + * Remove subscription if it couldn't finish all steps.
> + */
> +static void
> +drop_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
> +{
> +       PQExpBuffer str = createPQExpBuffer();
> +       PGresult   *res;
> +
> +       Assert(conn != NULL);
> +
> +       pg_log_info("dropping subscription \"%s\" on database \"%s\"",
> dbinfo->subname, dbinfo->dbname);
> +
> +       appendPQExpBuffer(str, "DROP SUBSCRIPTION %s",
> dbinfo->subname);
> +
> +       pg_log_debug("command is: %s", str->data);

Fixed.

> 4) I was not sure if drop_publication is required here, as we will not
> create any publication in subscriber node:
> +               if (dbinfo[i].made_subscription)
> +               {
> +                       conn = connect_database(dbinfo[i].subconninfo);
> +                       if (conn != NULL)
> +                       {
> +                               drop_subscription(conn, &dbinfo[i]);
> +                               if (dbinfo[i].made_publication)
> +                                       drop_publication(conn, &dbinfo[i]);
> +                               disconnect_database(conn);
> +                       }
> +               }

Removed. But I'm not sure the cleanup is really meaningful.
See [1].

> 5) The connection should be disconnected in case of error case:
> +       /* secure search_path */
> +       res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
> +       if (PQresultStatus(res) != PGRES_TUPLES_OK)
> +       {
> +               pg_log_error("could not clear search_path: %s",
> PQresultErrorMessage(res));
> +               return NULL;
> +       }
> +       PQclear(res);

PQfisnih() was added.

> 6) There should be a line break before postgres_fe inclusion, to keep
> it consistent:
> + *-------------------------------------------------------------------------
> + */
> +#include "postgres_fe.h"
> +
> +#include <signal.h>

Added.

> 7) These includes are not required:
> 7.a) #include <signal.h>
> 7.b) #include <sys/stat.h>
> 7.c) #include <sys/wait.h>
> 7.d) #include "access/xlogdefs.h"
> 7.e) #include "catalog/pg_control.h"
> 7.f) #include "common/file_utils.h"
> 7.g) #include "utils/pidfile.h"

Removed.

> + *             src/bin/pg_basebackup/pg_createsubscriber.c
> + *
> + *-------------------------------------------------------------------------
> + */
> +#include "postgres_fe.h"
> +
> +#include <signal.h>
> +#include <sys/stat.h>
> +#include <sys/time.h>
> +#include <sys/wait.h>
> +#include <time.h>
> +
> +#include "access/xlogdefs.h"
> +#include "catalog/pg_authid_d.h"
> +#include "catalog/pg_control.h"
> +#include "common/connect.h"
> +#include "common/controldata_utils.h"
> +#include "common/file_perm.h"
> +#include "common/file_utils.h"
> +#include "common/logging.h"
> +#include "common/restricted_token.h"
> +#include "fe_utils/recovery_gen.h"
> +#include "fe_utils/simple_list.h"
> +#include "getopt_long.h"
> +#include "utils/pidfile.h"
> 
> 8) POSTMASTER_STANDBY and POSTMASTER_FAILED are not being used, is it
> required or kept for future purpose:
> +enum WaitPMResult
> +{
> +       POSTMASTER_READY,
> +       POSTMASTER_STANDBY,
> +       POSTMASTER_STILL_STARTING,
> +       POSTMASTER_FAILED
> +};

I think they are here because WaitPMResult is just ported from pg_ctl.c.
I renamed the enumeration and removed non-necessary attributes.

> 9) pg_createsubscriber should be kept after pg_basebackup to maintain
> the consistent order:
> diff --git a/src/bin/pg_basebackup/.gitignore
> b/src/bin/pg_basebackup/.gitignore
> index 26048bdbd8..b3a6f5a2fe 100644
> --- a/src/bin/pg_basebackup/.gitignore
> +++ b/src/bin/pg_basebackup/.gitignore
> @@ -1,5 +1,6 @@
>  /pg_basebackup
>  /pg_receivewal
>  /pg_recvlogical
> +/pg_createsubscriber

Addressed.

> 10) dry-run help message is not very clear, how about something
> similar to pg_upgrade's message like "check clusters only, don't
> change any data":
> +       printf(_(" -d, --database=DBNAME               database to
> create a subscription\n"));
> +       printf(_(" -n, --dry-run                       stop before
> modifying anything\n"));
> +       printf(_(" -t, --recovery-timeout=SECS         seconds to wait
> for recovery to end\n"));
> +       printf(_(" -r, --retain                        retain log file
> after success\n"));
> +       printf(_(" -v, --verbose                       output verbose
> messages\n"));
> +       printf(_(" -V, --version                       output version
> information, then exit\n"));

Changed.

New patch is available in [2].

[1]:
https://www.postgresql.org/message-id/TYCPR01MB1207713BEC5C379A05D65E342F54B2%40TYCPR01MB12077.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/TYCPR01MB12077A6BB424A025F04A8243DF54F2%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear hackers,

I've replied for trackability.

> Further comments for v17.
> 
> 01.
> This program assumes that the target server has same major version with this.
> Because the target server would be restarted by same version's pg_ctl command.
> I felt it should be ensured by reading the PG_VERSION.

Still investigating.

> 02.
> pg_upgrade checked the version of using executables, like pg_ctl, postgres, and
> pg_resetwal. I felt it should be as well.

Still investigating.

> 03. get_bin_directory
> ```
>     if (find_my_exec(path, full_path) < 0)
>     {
>         pg_log_error("The program \"%s\" is needed by %s but was not
> found in the\n"
>                      "same directory as \"%s\".\n",
>                      "pg_ctl", progname, full_path);
> ```
> 
> s/"pg_ctl"/progname

The message was updated.

> 04.
> Missing canonicalize_path()?

I found find_my_exec() calls canonicalize_path(). No need to do.

> 05.
> Assuming that the target server is a cascade standby, i.e., it has a role as
> another primary. In this case, I thought the child node would not work. Because
> pg_createsubcriber runs pg_resetwal and all WAL files would be discarded at that
> time. I have not tested, but should the program detect it and exit earlier?

Still investigating.

> 06.
> wait_for_end_recovery() waits forever even if the standby has been disconnected
> from the primary, right? should we check the status of the replication via
> pg_stat_wal_receiver?

Still investigating.

> 07.
> The cleanup function has couple of bugs.
> 
> * If subscriptions have been created on the database, the function also tries to
>   drop a publication. But it leads an ERROR because it has been already dropped.
>   See setup_subscriber().
> * If the subscription has been created, drop_replication_slot() leads an ERROR.
>   Because the subscriber tried to drop the subscription while executing DROP
> SUBSCRIPTION.

Only drop_publication() was removed.

> 08.
> I found that all messages (ERROR, WARNING, INFO, etc...) would output to stderr,
> but I felt it should be on stdout. Is there a reason? pg_dump outputs messages to
> stderr, but the motivation might be to avoid confusion with dumps.

Still investigating.

> 09.
> I'm not sure the cleanup for subscriber is really needed. Assuming that there
> are two databases, e.g., pg1 pg2 , and we fail to create a subscription on pg2.
> This can happen when the subscription which has the same name has been
> already
> created on the primary server.
> In this case a subscirption pn pg1 would be removed. But what is a next step?
> Since a timelineID on the standby server is larger than the primary (note that
> the standby has been promoted once), we cannot resume the physical replication
> as-is. IIUC the easiest method to retry is removing a cluster once and restarting
> from pg_basebackup. If so, no need to cleanup the standby because it is
> corrupted.
> We just say "Please remove the cluster and recreate again".

I still think it should be, but not done yet.

New patch can be available in [1].

[1]:
https://www.postgresql.org/message-id/TYCPR01MB12077A6BB424A025F04A8243DF54F2%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear Shubham,

Thanks for testing the patch!

> 
> I tried verifying few scenarios by using 5 databases and came across
> the following errors:
> 
> ./pg_createsubscriber -D ../new_standby -P "host=localhost port=5432
> dbname=postgres" -S "host=localhost port=9000 dbname=postgres"  -d db1
> -d db2 -d db3 -d db4 -d db5
> 
> pg_createsubscriber: error: publisher requires 6 wal sender
> processes, but only 5 remain
> pg_createsubscriber: hint: Consider increasing max_wal_senders to at least 7.
> 
> It is successful only with 7 wal senders, so we can change error
> messages accordingly.
> 
> 
> pg_createsubscriber: error: publisher requires 6 replication slots,
> but only 5 remain
> pg_createsubscriber: hint: Consider increasing max_replication_slots
> to at least 7.
> 
> It is successful only with 7 replication slots, so we can change error
> messages accordingly.

I'm not a original author but I don't think it is needed. The hint message has
already suggested you to change to 7. According to the doc [1],  the primary
message should be factual and hint message should be used for suggestions. I felt
current code followed the style. Thought?

New patch is available in [2].

[1]: https://www.postgresql.org/docs/devel/error-style-guide.html
[2]:
https://www.postgresql.org/message-id/TYCPR01MB12077A6BB424A025F04A8243DF54F2%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

>

Remember the target server was a standby (read only access). I don't expect an

application trying to modify it; unless it is a buggy application.

>

What if the client modifies the data just after the promotion?

Naively considered, all the changes can be accepted, but are there any issues?

>

Regarding

GUCs, almost all of them is PGC_POSTMASTER (so it cannot be modified unless the

server is restarted). The ones that are not PGC_POSTMASTER, does not affect the

pg_createsubscriber execution [1].

>

IIUC,  primary_conninfo and primary_slot_name is PGC_SIGHUP.

>

I'm just pointing out that this case is a different from pg_upgrade (from which

this idea was taken). I'm not saying that's a bad idea. I'm just arguing that

you might be preventing some access read only access (monitoring) when it is

perfectly fine to connect to the database and execute queries. As I said

before, the current UI allows anyone to setup the standby to accept only local

connections. Of course, it is an extra step but it is possible. However, once

you apply v16-0007, there is no option but use only local connection during the

transformation. Is it an acceptable limitation?

>

My remained concern is written above. If they do not problematic we may not have

to restrict them for now. At that time, changes 

1) overwriting a port number,

2) setting listen_addresses = ''

are not needed, right? IIUC inconsistency of -P may be still problematic.

>

pglogical_create_subscriber does nothing [2][3].

>

Oh, thanks.

Just to confirm - pglogical set shared_preload_libraries to '', should we follow or not?

On Wed, 7 Feb 2024 at 10:24, Euler Taveira <euler@eulerto.com> wrote:

>

> On Fri, Feb 2, 2024, at 6:41 AM, Hayato Kuroda (Fujitsu) wrote:

>

> Thanks for updating the patch!

Thanks for the updated patch, few comments:

Few comments:

1) Cleanup function handler flag should be reset, i.e.

dbinfo->made_replslot = false; should be there else there will be an

error during  drop replication slot cleanup in error flow:

2) Cleanup function handler flag should be reset, i.e.

dbinfo->made_publication = false; should be there else there will be

an error during drop publication cleanup in error flow:

3) Cleanup function handler flag should be reset, i.e.

dbinfo->made_subscription = false; should be there else there will be

an error during drop publication cleanup in error flow:

4) I was not sure if drop_publication is required here, as we will not

create any publication in subscriber node:

+               if (dbinfo[i].made_subscription)

+               {

+                       conn = connect_database(dbinfo[i].subconninfo);

+                       if (conn != NULL)

+                       {

+                               drop_subscription(conn, &dbinfo[i]);

+                               if (dbinfo[i].made_publication)

+                                       drop_publication(conn, &dbinfo[i]);

+                               disconnect_database(conn);

+                       }

+               }

5) The connection should be disconnected in case of error case:

+       /* secure search_path */

+       res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);

+       if (PQresultStatus(res) != PGRES_TUPLES_OK)

+       {

+               pg_log_error("could not clear search_path: %s",

PQresultErrorMessage(res));

+               return NULL;

+       }

+       PQclear(res);

7) These includes are not required:

7.a) #include <signal.h>

7.b) #include <sys/stat.h>

7.c) #include <sys/wait.h>

7.d) #include "access/xlogdefs.h"

7.e) #include "catalog/pg_control.h"

7.f) #include "common/file_utils.h"

7.g) #include "utils/pidfile.h"

8) POSTMASTER_STANDBY and POSTMASTER_FAILED are not being used, is it

required or kept for future purpose:

+enum WaitPMResult

+{

+       POSTMASTER_READY,

+       POSTMASTER_STANDBY,

+       POSTMASTER_STILL_STARTING,

+       POSTMASTER_FAILED

+};

9) pg_createsubscriber should be kept after pg_basebackup to maintain

the consistent order:

10) dry-run help message is not very clear, how about something

similar to pg_upgrade's message like "check clusters only, don't

change any data":

Dear Euler,

>
If someone modifies data after promotion, fine; she has to deal with conflicts,
if any. IMO it is solved adding one or two sentences in the documentation.
>

OK. I could find issues, for now.

>
> >
> Regarding
> GUCs, almost all of them is PGC_POSTMASTER (so it cannot be modified unless the
> server is restarted). The ones that are not PGC_POSTMASTER, does not affect the
> pg_createsubscriber execution [1].
> >
> 
> IIUC,  primary_conninfo and primary_slot_name is PGC_SIGHUP.

Ditto.
>

Just to confirm - even if the primary_slot_name would be changed during
the conversion, the slot initially set would be dropped. Currently we do not
find any issues.

>
> >
> I'm just pointing out that this case is a different from pg_upgrade (from which
> this idea was taken). I'm not saying that's a bad idea. I'm just arguing that
> you might be preventing some access read only access (monitoring) when it is
> perfectly fine to connect to the database and execute queries. As I said
> before, the current UI allows anyone to setup the standby to accept only local
> connections. Of course, it is an extra step but it is possible. However, once
> you apply v16-0007, there is no option but use only local connection during the
> transformation. Is it an acceptable limitation?
> >
> 
> My remained concern is written above. If they do not problematic we may not have
> to restrict them for now. At that time, changes 
> 
> 1) overwriting a port number,
> 2) setting listen_addresses = ''

It can be implemented later if people are excited by it.

> are not needed, right? IIUC inconsistency of -P may be still problematic.

I still think we shouldn't have only the transformed primary_conninfo as
option.
>


Hmm, OK. So let me summarize current status and discussions. 

Policy)

Basically, we do not prohibit to connect to primary/standby.
primary_slot_name may be changed during the conversion and
tuples may be inserted on target just after the promotion, but it seems no issues.

API)

-D (data directory) and -d (databases) are definitively needed.

Regarding the -P (a connection string for source), we can require it for now.
But note that it may cause an inconsistency if the pointed not by -P is different
from the node pointde by primary_conninfo.

As for the connection string for the target server, we can choose two ways:
a)
accept native connection string as -S. This can reuse the same parsing mechanism as -P,
but there is a room that non-local server is specified.

b)
accept username/port as -U/-p
(Since the policy is like above, listen_addresses would not be overwritten. Also, the port just specify the listening
port).
This can avoid connecting to non-local, but more options may be needed.
(E.g., Is socket directory needed? What about password?)

Other discussing point, reported issue)

Points raised by me [1] are not solved yet.

* What if the target version is PG16-?
* What if the found executables have diffent version with pg_createsubscriber?
* What if the target is sending WAL to another server?
   I.e., there are clusters like `node1->node2-.node3`, and the target is node2.
* Can we really cleanup the standby in case of failure?
   Shouldn't we suggest to remove the target once?
* Can we move outputs to stdout?

[1]:
https://www.postgresql.org/message-id/TYCPR01MB1207713BEC5C379A05D65E342F54B2%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

On Thu, Feb 15, 2024 at 10:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Euler,
>
> Here are my minor comments for 17.
>
> 01.
> ```
> /* Options */
> static const char *progname;
>
> static char *primary_slot_name = NULL;
> static bool dry_run = false;
>
> static bool success = false;
>
> static LogicalRepInfo *dbinfo;
> static int      num_dbs = 0;
> ```
>
> The comment seems out-of-date. There is only one option.
>
> 02. check_subscriber and check_publisher
>
> Missing pg_catalog prefix in some lines.
>
> 03. get_base_conninfo
>
> I think dbname would not be set. IIUC, dbname should be a pointer of the pointer.
>
>
> 04.
>
> I check the coverage and found two functions have been never called:
>  - drop_subscription
>  - drop_replication_slot
>
> Also, some cases were not tested. Below bullet showed notable ones for me.
> (Some of them would not be needed based on discussions)
>
> * -r is specified
> * -t is specified
> * -P option contains dbname
> * -d is not specified
> * GUC settings are wrong
> * primary_slot_name is specified on the standby
> * standby server is not working
>
> In feature level, we may able to check the server log is surely removed in case
> of success.
>
> So, which tests should be added? drop_subscription() is called only when the
> cleanup phase, so it may be difficult to test. According to others, it seems that
> -r and -t are not tested. GUC-settings have many test cases so not sure they
> should be. Based on this, others can be tested.
>
> PSA my top-up patch set.
>
> V18-0001: same as your patch, v17-0001.
> V18-0002: modify the alignment of codes.
> V18-0003: change an argument of get_base_conninfo. Per comment 3.
> === experimental patches ===
> V18-0004: Add testcases per comment 4.
> V18-0005: Remove -P option. I'm not sure it should be needed, but I made just in case.

I created a cascade Physical Replication system like
node1->node2->node3 and ran pg_createsubscriber for node2. After
running the script, I started the node2 again and found
pg_createsubscriber command was successful after which the physical
replication between node2 and node3 has been broken. I feel
pg_createsubscriber should check this scenario and throw an error in
this case to avoid breaking the cascaded replication setup. I have
attached the script which was used to verify this.

Thanks and Regards,
Shubham Khanna.

Dear Euler, 

> Policy)
> 
> Basically, we do not prohibit to connect to primary/standby.
> primary_slot_name may be changed during the conversion and
> tuples may be inserted on target just after the promotion, but it seems no issues.
> 
> API)
> 
> -D (data directory) and -d (databases) are definitively needed.
> 
> Regarding the -P (a connection string for source), we can require it for now.
> But note that it may cause an inconsistency if the pointed not by -P is different
> from the node pointde by primary_conninfo.
> 
> As for the connection string for the target server, we can choose two ways:
> a)
> accept native connection string as -S. This can reuse the same parsing
> mechanism as -P,
> but there is a room that non-local server is specified.
> 
> b)
> accept username/port as -U/-p
> (Since the policy is like above, listen_addresses would not be overwritten. Also,
> the port just specify the listening port).
> This can avoid connecting to non-local, but more options may be needed.
> (E.g., Is socket directory needed? What about password?)
> 
> Other discussing point, reported issue)
> 
> Points raised by me [1] are not solved yet.
> 
> * What if the target version is PG16-?
> * What if the found executables have diffent version with pg_createsubscriber?
> * What if the target is sending WAL to another server?
>    I.e., there are clusters like `node1->node2-.node3`, and the target is node2.
> * Can we really cleanup the standby in case of failure?
>    Shouldn't we suggest to remove the target once?
> * Can we move outputs to stdout?

Based on the discussion, I updated the patch set. Feel free to pick them and include.
Removing -P patch was removed, but removing -S still remained.

Also, while testing the patch set, I found some issues.

1.
Cfbot got angry [1]. This is because WIFEXITED and others are defined in <sys/wait.h>,
but the inclusion was removed per comment. Added the inclusion again.

2.
As Shubham pointed out [3], when we convert an intermediate node of cascading replication,
the last node would stuck. This is because a walreciever process requires nodes have the same
system identifier (in WalReceiverMain), but it would be changed by pg_createsubscriebr.

3.
Moreover, when we convert a last node of cascade, it won't work well. Because we cannot create
publications on the standby node.

4.
If the standby server was initialized as PG16-, this command would fail.
Because the API of pg_logical_create_replication_slot() were changed.

5.
Also, used pg_ctl commands must have same versions with the instance.
I think we should require all the executables and servers must be a same major version.

Based on them, below part describes attached ones:

V20-0001: same as Euler's patch, v17-0001.
V20-0002: Update docs per recent changes. Same as v19-0002
V20-0003: Modify the alignment of codes. Same as v19-0003
V20-0004: Change an argument of get_base_conninfo. Same as v19-0004
=== experimental patches ===
V20-0005: Add testcases. Same as v19-0004
V20-0006: Update a comment above global variables. Same as v19-0005
V20-0007: Address comments from Vignesh. Some parts you don't like
          are reverted.
V20-0008: Fix error message in get_bin_directory(). Same as v19-0008
V20-0009: Remove -S option. Refactored from v16-0007
V20-0010: Add check versions of executables and the target, per above and [4]
V20-0011: Detect a disconnection while waiting the recovery, per [4]
V20-0012: Avoid running pg_createsubscriber for cascade physical replication, per above.

[1]: https://cirrus-ci.com/task/4619792833839104
[2]: https://www.postgresql.org/message-id/CALDaNm1r9ZOwZamYsh6MHzb%3D_XvhjC_5XnTAsVecANvU9FOz6w%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAHv8RjJcUY23ieJc5xqg6-QeGr1Ppp4Jwbu7Mq29eqCBTDWfUw%40mail.gmail.com
[4]:
https://www.postgresql.org/message-id/TYCPR01MB1207713BEC5C379A05D65E342F54B2%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

> Points raised by me [1] are not solved yet.

> 

> * What if the target version is PG16-?

> * What if the found executables have diffent version with pg_createsubscriber?

> * What if the target is sending WAL to another server?

>    I.e., there are clusters like `node1->node2-.node3`, and the target is node2.

> * Can we really cleanup the standby in case of failure?

>    Shouldn't we suggest to remove the target once?

> * Can we move outputs to stdout?

1.

Cfbot got angry [1]. This is because WIFEXITED and others are defined in <sys/wait.h>,

but the inclusion was removed per comment. Added the inclusion again.

2.

As Shubham pointed out [3], when we convert an intermediate node of cascading replication,

the last node would stuck. This is because a walreciever process requires nodes have the same

system identifier (in WalReceiverMain), but it would be changed by pg_createsubscriebr.

3.

Moreover, when we convert a last node of cascade, it won't work well. Because we cannot create

publications on the standby node.

4.

If the standby server was initialized as PG16-, this command would fail.

Because the API of pg_logical_create_replication_slot() were changed.

5.

Also, used pg_ctl commands must have same versions with the instance.

I think we should require all the executables and servers must be a same major version.

Based on them, below part describes attached ones:

Dear Euler,

Thanks for updating the patch!
Before reviewing deeply, here are replies for your comments.

>
> > Points raised by me [1] are not solved yet.
> > 
> > * What if the target version is PG16-?
pg_ctl and pg_resetwal won't work.
$ pg_ctl start -D /tmp/blah
waiting for server to start....
2024-02-15 23:50:03.448 -03 [364610] FATAL:  database files are incompatible with server
2024-02-15 23:50:03.448 -03 [364610] DETAIL:  The data directory was initialized by PostgreSQL version 16, which is not
compatiblewith this version 17devel.
 
stopped waiting
pg_ctl: could not start server
Examine the log output.

$ pg_resetwal -D /tmp/blah
pg_resetwal: error: data directory is of wrong version
pg_resetwal: detail: File "PG_VERSION" contains "16", which is not compatible with this program's version "17".

> > * What if the found executables have diffent version with pg_createsubscriber?

The new code take care of it.
>

I preferred to have a common path and test one by one, but agreed this worked well.
Let's keep it and hear opinions from others.

>
> > * What if the target is sending WAL to another server?
> >    I.e., there are clusters like `node1->node2-.node3`, and the target is node2.
The new code detects if the server is in recovery and aborts as you suggested.
A new option can be added to ignore the fact there are servers receiving WAL
from it.
>

Confirmed it can detect.

>
> > * Can we really cleanup the standby in case of failure?
> >    Shouldn't we suggest to remove the target once?

If it finishes the promotion, no. I adjusted the cleanup routine a bit to avoid
it. However, we should provide instructions to inform the user that it should
create a fresh standby and try again.
>

I think the cleanup function looks not sufficient. In v21, recovery_ended is kept
to true even after drop_publication() is done in setup_subscriber(). I think that
made_subscription is not needed, and the function should output some messages
when recovery_ended is on.
Besides, in case of pg_upgrade, they always report below messages before starting
the migration. I think this is more helpful for users.

```
    pg_log(PG_REPORT, "\n"
           "If pg_upgrade fails after this point, you must re-initdb the\n"
           "new cluster before continuing.");
```

>
> > * Can we move outputs to stdout?

Are you suggesting to use another logging framework? It is not a good idea
because each client program is already using common/logging.c.
>

Hmm, indeed. Other programs in pg_basebackup seem to use the framework.

>
v20-0011: Do we really want to interrupt the recovery if the network was
momentarily interrupted or if the OS killed walsender? Recovery is critical for
the process. I think we should do our best to be resilient and deliver all
changes required by the new subscriber.
>

It might be too strict to raise an ERROR as soon as we met a disconnection.
And at least 0011 was wrong - it should be PQgetvalue(res, 0, 1) for still_alive.

>
The proposal is not correct because the
query return no tuples if it is disconnected so you cannot PQgetvalue().
>

Sorry for misunderstanding, but you might be confused. pg_createsubcriber
sends a query to target, and we are discussing the disconnection between the
target and source instances. I think the connection which pg_createsubscriber
has is stil alive so PQgetvalue() can get a value.

(BTW, callers of server_is_in_recovery() has not considered a disconnection from
the target...)

>
The
retry interval (the time that ServerLoop() will create another walreceiver) is
determined by DetermineSleepTime() and it is a maximum of 5 seconds
(SIGKILL_CHILDREN_AFTER_SECS). One idea is to retry 2 or 3 times before give up
using the pg_stat_wal_receiver query. Do you have a better plan?
>

It's good to determine the threshold. It can define the number  of acceptable
loss of walreceiver during the loop.
I think we should retry at least the postmaster revives the walreceiver.
The checking would work once per seconds, so more than 5 (or 10) may be better.
Thought?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

On Thu, Feb 15, 2024 at 4:53 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Euler,
>
> > Policy)
> >
> > Basically, we do not prohibit to connect to primary/standby.
> > primary_slot_name may be changed during the conversion and
> > tuples may be inserted on target just after the promotion, but it seems no issues.
> >
> > API)
> >
> > -D (data directory) and -d (databases) are definitively needed.
> >
> > Regarding the -P (a connection string for source), we can require it for now.
> > But note that it may cause an inconsistency if the pointed not by -P is different
> > from the node pointde by primary_conninfo.
> >
> > As for the connection string for the target server, we can choose two ways:
> > a)
> > accept native connection string as -S. This can reuse the same parsing
> > mechanism as -P,
> > but there is a room that non-local server is specified.
> >
> > b)
> > accept username/port as -U/-p
> > (Since the policy is like above, listen_addresses would not be overwritten. Also,
> > the port just specify the listening port).
> > This can avoid connecting to non-local, but more options may be needed.
> > (E.g., Is socket directory needed? What about password?)
> >
> > Other discussing point, reported issue)
> >
> > Points raised by me [1] are not solved yet.
> >
> > * What if the target version is PG16-?
> > * What if the found executables have diffent version with pg_createsubscriber?
> > * What if the target is sending WAL to another server?
> >    I.e., there are clusters like `node1->node2-.node3`, and the target is node2.
> > * Can we really cleanup the standby in case of failure?
> >    Shouldn't we suggest to remove the target once?
> > * Can we move outputs to stdout?
>
> Based on the discussion, I updated the patch set. Feel free to pick them and include.
> Removing -P patch was removed, but removing -S still remained.
>
> Also, while testing the patch set, I found some issues.
>
> 1.
> Cfbot got angry [1]. This is because WIFEXITED and others are defined in <sys/wait.h>,
> but the inclusion was removed per comment. Added the inclusion again.
>
> 2.
> As Shubham pointed out [3], when we convert an intermediate node of cascading replication,
> the last node would stuck. This is because a walreciever process requires nodes have the same
> system identifier (in WalReceiverMain), but it would be changed by pg_createsubscriebr.
>
> 3.
> Moreover, when we convert a last node of cascade, it won't work well. Because we cannot create
> publications on the standby node.
>
> 4.
> If the standby server was initialized as PG16-, this command would fail.
> Because the API of pg_logical_create_replication_slot() were changed.
>
> 5.
> Also, used pg_ctl commands must have same versions with the instance.
> I think we should require all the executables and servers must be a same major version.
>
> Based on them, below part describes attached ones:
>
> V20-0001: same as Euler's patch, v17-0001.
> V20-0002: Update docs per recent changes. Same as v19-0002
> V20-0003: Modify the alignment of codes. Same as v19-0003
> V20-0004: Change an argument of get_base_conninfo. Same as v19-0004
> === experimental patches ===
> V20-0005: Add testcases. Same as v19-0004
> V20-0006: Update a comment above global variables. Same as v19-0005
> V20-0007: Address comments from Vignesh. Some parts you don't like
>           are reverted.
> V20-0008: Fix error message in get_bin_directory(). Same as v19-0008
> V20-0009: Remove -S option. Refactored from v16-0007
> V20-0010: Add check versions of executables and the target, per above and [4]
> V20-0011: Detect a disconnection while waiting the recovery, per [4]
> V20-0012: Avoid running pg_createsubscriber for cascade physical replication, per above.
>
> [1]: https://cirrus-ci.com/task/4619792833839104
> [2]: https://www.postgresql.org/message-id/CALDaNm1r9ZOwZamYsh6MHzb%3D_XvhjC_5XnTAsVecANvU9FOz6w%40mail.gmail.com
> [3]: https://www.postgresql.org/message-id/CAHv8RjJcUY23ieJc5xqg6-QeGr1Ppp4Jwbu7Mq29eqCBTDWfUw%40mail.gmail.com
> [4]:
https://www.postgresql.org/message-id/TYCPR01MB1207713BEC5C379A05D65E342F54B2%40TYCPR01MB12077.jpnprd01.prod.outlook.com

I found a couple of issues, while verifying the cascaded replication
with the following scenarios:
Scenario 1) Create cascade replication like node1->node2->node3
without using replication slots (attached
cascade_3node_setup_without_slots has the script for this):
Then I ran pg_createsubscriber by specifying primary as node1 and
standby as node3, this scenario runs successfully. I was not sure if
this should be supported or not?
Scenario 2) Create cascade replication like node1->node2->node3 using
replication slots (attached cascade_3node_setup_with_slots has the
script for this):
Here, slot name was used as slot1 for node1 to node2 and slot2 for
node2 to node3. Then I ran pg_createsubscriber by specifying primary
as node1 and standby as node3. In this case pg_createsubscriber fails
with the following error:
pg_createsubscriber: error: could not obtain replication slot
information: got 0 rows, expected 1 row
[Inferior 1 (process 2623483) exited with code 01]

This is failing because slot name slot2 is used between node2->node3
but pg_createsubscriber is checked for slot1, the slot which is used
for replication between node1->node2.
Thoughts?

Thanks and Regards,
Shubham Khanna.

Dear Shubham,

Thanks for testing. It seems you ran with v20 patch, but I confirmed
It could reproduce with v21.

> 
> I found a couple of issues, while verifying the cascaded replication
> with the following scenarios:
> Scenario 1) Create cascade replication like node1->node2->node3
> without using replication slots (attached
> cascade_3node_setup_without_slots has the script for this):
> Then I ran pg_createsubscriber by specifying primary as node1 and
> standby as node3, this scenario runs successfully. I was not sure if
> this should be supported or not?

Hmm. After the script, the cascading would be broken. The replication would be:

```
Node1 -> node2
  |
Node3
```

And the operation is bit strange. The consistent LSN is gotten from the node1,
but node3 waits until it receives the record from NODE2.
Can we always success it?

> Scenario 2) Create cascade replication like node1->node2->node3 using
> replication slots (attached cascade_3node_setup_with_slots has the
> script for this):
> Here, slot name was used as slot1 for node1 to node2 and slot2 for
> node2 to node3. Then I ran pg_createsubscriber by specifying primary
> as node1 and standby as node3. In this case pg_createsubscriber fails
> with the following error:
> pg_createsubscriber: error: could not obtain replication slot
> information: got 0 rows, expected 1 row
> [Inferior 1 (process 2623483) exited with code 01]
> 
> This is failing because slot name slot2 is used between node2->node3
> but pg_createsubscriber is checked for slot1, the slot which is used
> for replication between node1->node2.
> Thoughts?

Right. The inconsistency is quite strange.

Overall, I felt such a case must be rejected. How should we detect at checking phase?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear Euler,

Here are comments for v21.

01. main
```
    /* rudimentary check for a data directory. */
...
    /* subscriber PID file. */
```

Initial char must be upper, and period is not needed.

02. check_data_directory
```
    snprintf(versionfile, MAXPGPATH, "%s/PG_VERSION", datadir);
```

You removed the version checking from PG_VERSION, but I think it is still needed.
Indeed v21 can detect the pg_ctl/pg_resetwal/pg_createsubscriber has different
verson, but this cannot ditect the started instance has the differnet version.
I.e., v20-0010 is partially needed.

03. store_pub_sub_info()
```
    SimpleStringListCell *cell;
```

This definition can be in loop variable.

04. get_standby_sysid()
```
    pfree(cf);
```

This can be pg_pfree().

05. check_subscriber
```
    /* The target server must be a standby */
    if (server_is_in_recovery(conn) == 0)
    {
        pg_log_error("The target server is not a standby");
        return false;
    }
```

What if the the function returns -1? Should we ditect (maybe the disconnection) here?

06. server_is_in_recovery
```
    ret = strcmp("t", PQgetvalue(res, 0, 0));

    PQclear(res);

    if (ret == 0)
        return 1;
    else if (ret > 0)
        return 0;
    else
        return -1;                /* should not happen */
```

But strcmp may return a negative value, right? Based on the comment atop function,
we should not do it. I think we can use ternary operator instead.

07. server_is_in_recovery

As the fisrt place, no one consider this returns -1. So can we change the bool
function and raise pg_fatal() in case of the error?

08. check_subscriber
```
    if (strcmp(PQgetvalue(res, 0, 1), "t") != 0)
    {
        pg_log_error("permission denied for function \"%s\"",
                     "pg_catalog.pg_replication_origin_advance(text, pg_lsn)");
        return false;
    }
```

I think the third argument must be 2.

09. check_subscriber
```
    pg_log_debug("subscriber: primary_slot_name: %s", primary_slot_name);
```

The output seems strange if the primary_slot_name is not set.

10. setup_publisher()
```
    PGconn       *conn;
    PGresult   *res;
```

Definitions can be in the loop.

11. create_publication()
```
    if (PQntuples(res) == 1)
    {
        /*
         * If publication name already exists and puballtables is true, let's
         * use it. A previous run of pg_createsubscriber must have created
         * this publication. Bail out.
         */
```

Hmm, but pre-existing publications may not send INSERT/UPDATE/DELETE/TRUNCATE.
They should be checked if we really want to reuse.
(I think it is OK to just raise ERROR)

12. create_publication()

Based on above, we do not have to check before creating publicatios. The publisher
can detect the duplication. I prefer it.

13. create_logical_replication_slot()
```
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
        {
            pg_log_error("could not create replication slot \"%s\" on database \"%s\": %s",
                         slot_name, dbinfo->dbname,
                         PQresultErrorMessage(res));
            return lsn;
        }
```

I know lsn is always NULL, but can we use `return NULL`?

14. setup_subscriber()
```
    PGconn       *conn;

```

This definition can be in the loop.


15.

You said in case of failure, cleanups is not needed if the process exits soon [1].
But some functions call PQfinish() then exit(1) or pg_fatal(). Should we follow?

16.

Some places refer PGresult or PGConn even after the cleanup. They must be fixed.
```
        PQclear(res);
        disconnect_database(conn);
        pg_fatal("could not get system identifier: %s",
                 PQresultErrorMessage(res));
```

I think this is a root cause why sometimes the wrong error message has output.


17.

Some places call PQerrorMessage() and other places call PQresultErrorMessage().
I think it PQerrorMessage() should be used only after the connection establishment
functions. Thought?

18. 041_pg_createsubscriber_standby.pl
```
use warnings;
```

We must set "FATAL = all";

19.
```
my $node_p;
my $node_f;
my $node_s;
my $node_c;
my $result;
my $slotname;
```

I could not find forward declarations in perl file.
The node name might be bit a consuging, but I could not find better name.

20.
```
# On node P
# - create databases
# - create test tables
# - insert a row
# - create a physical replication slot
$node_p->safe_psql(
    'postgres', q(
    CREATE DATABASE pg1;
    CREATE DATABASE pg2;
));
$node_p->safe_psql('pg1', 'CREATE TABLE tbl1 (a text)');
$node_p->safe_psql('pg1', "INSERT INTO tbl1 VALUES('first row')");
$node_p->safe_psql('pg2', 'CREATE TABLE tbl2 (a text)');
my $slotname = 'physical_slot';
$node_p->safe_psql('pg2',
    "SELECT pg_create_physical_replication_slot('$slotname')");
```

I think setting of the same node can be gathered into one place.
Also, any settings and definitions should be done just before they are used.

21.
```
$node_s->append_conf(
    'postgresql.conf', qq[
log_min_messages = debug2
primary_slot_name = '$slotname'
]);
```

I could not find a reason why we set to debug2.

22.
```
command_fails
```

command_checks_all() can check returned value and outputs.
Should we use it?

23.

Can you add headers for each testcases? E.g., 

```
# ------------------------------
# Check pg_createsubscriber fails when the target server is not a
# standby of the source.
...
# ------------------------------
# Check pg_createsubscriber fails when the target server is not running
...
# ------------------------------
# Check pg_createsubscriber fails when the target server is a member of
# the cascading standby.
...
# ------------------------------
# Check successful dry-run 
...
# ------------------------------
# Check successful conversion
```

24.
```
# Stop node C
$node_c->teardown_node;
...
$node_p->stop;
$node_s->stop;
$node_f->stop;
```
Why you choose the teardown?

25.

The creation of subscriptions are not directory tested. @subnames contains the
name of subscriptions, but it just assumes the number of them is more than two.

Since it may be useful, I will post top-up patch on Monday, if there are no updating.

[1]: https://www.postgresql.org/message-id/89ccf72b-59f9-4317-b9fd-1e6d20a0c3b1%40app.fastmail.com
[2]:
https://www.postgresql.org/message-id/TYCPR01MB1207713BEC5C379A05D65E342F54B2%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/ 

Dear hackers,

> Since it may be useful, I will post top-up patch on Monday, if there are no
> updating.

And here are top-up patches. Feel free to check and include.

v22-0001: Same as v21-0001.
=== rebased patches ===
v22-0002: Update docs per recent changes. Same as v20-0002.
v22-0003: Add check versions of the target. Extracted from v20-0003.
v22-0004: Remove -S option. Mostly same as v20-0009, but commit massage was
          slightly changed.
=== Newbie ===
V22-0005: Addressed my comments which seems to be trivial[1].
          Comments #1, 3, 4, 8, 10, 14, 17 were addressed here.
v22-0006: Consider the scenario when commands are failed after the recovery.
          drop_subscription() is removed and some messages are added per [2].
V22-0007: Revise server_is_in_recovery() per [1]. Comments #5, 6, 7, were addressed here.
V22-0008: Fix a strange report when physical_primary_slot is null. Per comment #9 [1].
V22-0009: Prohibit reuse publications when it has already existed. Per comments #11 and 12 [1].
V22-0010: Avoid to call PQclear()/PQfinish()/pg_free() if the process exits soon. Per comment #15 [1].
V22-0011: Update testcode. Per comments #17- [1].

I did not handle below points because I have unclear points.

a. 
This patch set cannot detect the disconnection between the target (standby) and
source (primary) during the catch up. Because the connection status must be gotten
at the same time (=in the same query) with the recovery status, but now it is now an
independed function (server_is_in_recovery()).

b.
This patch set cannot detect the inconsistency reported by Shubham [3]. I could not
come up with solutions without removing -P...

[1]:
https://www.postgresql.org/message-id/TYCPR01MB12077756323B79042F29DDAEDF54C2%40TYCPR01MB12077.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/TYCPR01MB12077E98F930C3DE6BD304D0DF54C2%40TYCPR01MB12077.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/message-id/CAHv8Rj%2B5mzK9Jt%2B7ECogJzfm5czvDCCd5jO1_rCx0bTEYpBE5g%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Some review of the v21 patch:

- commit message

Mention pg_createsubscriber in the commit message title.  That's the 
most important thing that someone doing git log searches in the future 
will be looking for.


- doc/src/sgml/ref/allfiles.sgml

Move the new entry to alphabetical order.


- doc/src/sgml/ref/pg_createsubscriber.sgml

+  <para>
+   The <application>pg_createsubscriber</application> should be run at 
the target
+   server. The source server (known as publisher server) should accept 
logical
+   replication connections from the target server (known as subscriber 
server).
+   The target server should accept local logical replication connection.
+  </para>

"should" -> "must" ?

+ <refsect1>
+  <title>Options</title>

Sort options alphabetically.

It would be good to indicate somewhere which options are mandatory.

+ <refsect1>
+  <title>Examples</title>

I suggest including a pg_basebackup call into this example, so it's
easier for readers to get the context of how this is supposed to be
used.  You can add that pg_basebackup in this example is just an example 
and that other base backups can also be used.


- doc/src/sgml/reference.sgml

Move the new entry to alphabetical order.


- src/bin/pg_basebackup/Makefile

Move the new sections to alphabetical order.


- src/bin/pg_basebackup/meson.build

Move the new sections to alphabetical order.


- src/bin/pg_basebackup/pg_createsubscriber.c

+typedef struct CreateSubscriberOptions
+typedef struct LogicalRepInfo

I think these kinds of local-use struct don't need to be typedef'ed.
(Then you also don't need to update typdefs.list.)

+static void
+usage(void)

Sort the options alphabetically.

+static char *
+get_exec_path(const char *argv0, const char *progname)

Can this not use find_my_exec() and find_other_exec()?

+int
+main(int argc, char **argv)

Sort the options alphabetically (long_options struct, getopt_long()
argument, switch cases).


- .../t/040_pg_createsubscriber.pl
- .../t/041_pg_createsubscriber_standby.pl

These two files could be combined into one.

+# Force it to initialize a new cluster instead of copying a
+# previously initdb'd cluster.

Explain why?

+$node_s->append_conf(
+    'postgresql.conf', qq[
+log_min_messages = debug2

Is this setting necessary for the test?

Hi,

I have reviewed the v21 patch. And found an issue.

Initially I started the standby server with a new postgresql.conf file
(not the default postgresql.conf that is present in the instance).
pg_ctl -D ../standby start -o "-c config_file=/new_path/postgresql.conf"

And I have made 'max_replication_slots = 1' in new postgresql.conf and
made  'max_replication_slots = 0' in the default postgresql.conf file.
Now when we run pg_createsubscriber on standby we get error:
pg_createsubscriber: error: could not set replication progress for the
subscription "pg_createsubscriber_5_242843": ERROR:  cannot query or
manipulate replication origin when max_replication_slots = 0
NOTICE:  dropped replication slot "pg_createsubscriber_5_242843" on publisher
pg_createsubscriber: error: could not drop publication
"pg_createsubscriber_5" on database "postgres": ERROR:  publication
"pg_createsubscriber_5" does not exist
pg_createsubscriber: error: could not drop replication slot
"pg_createsubscriber_5_242843" on database "postgres": ERROR:
replication slot "pg_createsubscriber_5_242843" does not exist

I observed that when we run the pg_createsubscriber command, it will
stop the standby instance (the non-default postgres configuration) and
restart the standby instance which will now be started with default
postgresql.conf, where the 'max_replication_slot = 0' and
pg_createsubscriber will now fail with the error given above.
I have added the script file with which we can reproduce this issue.
Also similar issues can happen with other configurations such as port, etc.

The possible solution would be
1) allow to run pg_createsubscriber if standby is initially stopped .
I observed that pg_logical_createsubscriber also uses this approach.
2) read GUCs via SHOW command and restore them when server restarts

I would prefer the 1st solution.

Thanks and Regards,
Shlok Kyal

Some review of the v21 patch:

- commit message

Mention pg_createsubscriber in the commit message title.  That's the 

most important thing that someone doing git log searches in the future 

will be looking for.

- doc/src/sgml/ref/allfiles.sgml

Move the new entry to alphabetical order.

- doc/src/sgml/ref/pg_createsubscriber.sgml

+  <para>

+   The <application>pg_createsubscriber</application> should be run at 

the target

+   server. The source server (known as publisher server) should accept 

logical

+   replication connections from the target server (known as subscriber 

server).

+   The target server should accept local logical replication connection.

+  </para>

"should" -> "must" ?

+ <refsect1>

+  <title>Options</title>

Sort options alphabetically.

It would be good to indicate somewhere which options are mandatory.

+ <refsect1>

+  <title>Examples</title>

I suggest including a pg_basebackup call into this example, so it's

easier for readers to get the context of how this is supposed to be

used.  You can add that pg_basebackup in this example is just an example 

and that other base backups can also be used.

- doc/src/sgml/reference.sgml

Move the new entry to alphabetical order.

- src/bin/pg_basebackup/Makefile

Move the new sections to alphabetical order.

- src/bin/pg_basebackup/meson.build

Move the new sections to alphabetical order.

- src/bin/pg_basebackup/pg_createsubscriber.c

+typedef struct CreateSubscriberOptions

+typedef struct LogicalRepInfo

I think these kinds of local-use struct don't need to be typedef'ed.

(Then you also don't need to update typdefs.list.)

+static void

+usage(void)

Sort the options alphabetically.

+static char *

+get_exec_path(const char *argv0, const char *progname)

Can this not use find_my_exec() and find_other_exec()?

+int

+main(int argc, char **argv)

Sort the options alphabetically (long_options struct, getopt_long()

argument, switch cases).

- .../t/040_pg_createsubscriber.pl

- .../t/041_pg_createsubscriber_standby.pl

These two files could be combined into one.

+# Force it to initialize a new cluster instead of copying a

+# previously initdb'd cluster.

Explain why?

+$node_s->append_conf(

+ 'postgresql.conf', qq[

+log_min_messages = debug2

Is this setting necessary for the test?

I have reviewed the v21 patch. And found an issue.

Initially I started the standby server with a new postgresql.conf file

(not the default postgresql.conf that is present in the instance).

pg_ctl -D ../standby start -o "-c config_file=/new_path/postgresql.conf"

And I have made 'max_replication_slots = 1' in new postgresql.conf and

made  'max_replication_slots = 0' in the default postgresql.conf file.

Now when we run pg_createsubscriber on standby we get error:

pg_createsubscriber: error: could not set replication progress for the

subscription "pg_createsubscriber_5_242843": ERROR:  cannot query or

manipulate replication origin when max_replication_slots = 0

NOTICE:  dropped replication slot "pg_createsubscriber_5_242843" on publisher

pg_createsubscriber: error: could not drop publication

"pg_createsubscriber_5" on database "postgres": ERROR:  publication

"pg_createsubscriber_5" does not exist

pg_createsubscriber: error: could not drop replication slot

"pg_createsubscriber_5_242843" on database "postgres": ERROR:

replication slot "pg_createsubscriber_5_242843" does not exist