Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.

On Mon, Mar 24, 2025 at 6:08 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, Mar 21, 2025 at 6:59 PM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> > > <khannashubham1197(at)gmail(dot)com> wrote:
> > >
> > > >
> > > > The attached patches contain the suggested changes.
> > > >
> > >
> > > I have started reviewing the patches again. Here are some review comments
> > >
> > > <variablelist>
> > > + <varlistentry>
> > > + <term><option>-a</option></term>
> > > + <term><option>--all</option></term>
> > > + <listitem>
> > > + <para>
> > > + For all source server non-template databases create subscriptions for
> > > + databases with the same names on the target server.
> > >
> > > In this construct "all" goes with "source server" but it should go
> > > with "non-template database". How about, "For every non-template
> > > database on the source server, create one subscription on the target
> > > server in the database with the same name."?
> > >
> >
> > Fixed.
> >
> > > + Subscription names, publication names, and replication slot names are
> > > + automatically generated. This option cannot be used together with
> > > + <option>--database</option>, <option>--publication</option>,
> > > + <option>--replication-slot</option> or <option>--subscription</option>.
> > >
> > > ... used along with ...
> > >
> > > also comma before or .
> > >
> > > We should also mention that generated names of replication slots,
> > > publications and subscriptions are used when using this option.
> > >
> > > + </para>
> > > + </listitem>
> > > + </varlistentry>
> > > +
> > > <varlistentry>
> > > <term><option>-d <replaceable
> > > class="parameter">dbname</replaceable></option></term>
> > > <term><option>--database=<replaceable
> > > class="parameter">dbname</replaceable></option></term>
> >
> > Fixed.
> >
> > > @@ -94,9 +130,10 @@ PostgreSQL documentation
> > > <para>
> > > The name of the database in which to create a subscription. Multiple
> > > databases can be selected by writing multiple <option>-d</option>
> > > - switches. If <option>-d</option> option is not provided, the database
> > > - name will be obtained from <option>-P</option> option. If the database
> > > - name is not specified in either the <option>-d</option> option or
> > > + switches. This option cannot be used together with
> > > <option>--all</option>.
> > > + If <option>-d</option> option is not provided, the database name will be
> > > + obtained from <option>-P</option> option. If the database name is not
> > > + specified in either the <option>-d</option> option or
> > > <option>-P</option> option, an error will be reported.
> > >
> > > We have to mention -a as well here. Something like "If the database
> > > name is not specified in either the <option>-d</option> option or
> > > <option>-P</option> option, and <option>-a</option> is not provided,
> > > an error will be reported."
> > >
> >
> > Fixed.
> >
> > > +# run pg_createsubscriber with '--database' and '--all' without '--dry-run'
> > > +# and verify the failure
> > > +command_fails_like(
> > > + [
> > > + 'pg_createsubscriber',
> > > + '--verbose',
> > > + '--pgdata' => $node_s->data_dir,
> > > + '--publisher-server' => $node_p->connstr($db1),
> > > + '--socketdir' => $node_s->host,
> > > + '--subscriber-port' => $node_s->port,
> > > + '--database' => $db1,
> > > + '--all',
> > > + ],
> > > + qr/--database cannot be used with --all/,
> > > + 'fail if --database is used with --all');
> > >
> > >
> > > Why does only this test not use --dry-run, but all other such tests
> > > use --dry-run? Either they should all use it or not use it.
> > >
> > > +
> > > +# run pg_createsubscriber with '--publication' and '--all' and verify
> > > +# the failure
> > > +command_fails_like(
> > > + [
> > > + 'pg_createsubscriber',
> > > + '--verbose',
> > > + '--dry-run',
> > > + '--pgdata' => $node_s->data_dir,
> > > + '--publisher-server' => $node_p->connstr($db1),
> > > + '--socketdir' => $node_s->host,
> > > + '--subscriber-port' => $node_s->port,
> > > + '--all',
> > > + '--publication' => 'pub1',
> > > + ],
> > > + qr/--publication cannot be used with --all/,
> > > + 'fail if --publication is used with --all');
> > > +
> > > +# run pg_createsubscriber with '--replication-slot' and '--all' and
> > > +# verify the failure
> > > +command_fails_like(
> > > + [
> > > + 'pg_createsubscriber',
> > > + '--verbose',
> > > + '--dry-run',
> > > + '--pgdata' => $node_s->data_dir,
> > > + '--publisher-server' => $node_p->connstr($db1),
> > > + '--socketdir' => $node_s->host,
> > > + '--subscriber-port' => $node_s->port,
> > > + '--replication-slot' => 'replslot1',
> > > + '--all',
> > > + ],
> > > + qr/--replication-slot cannot be used with --all/,
> > > + 'fail if --replication-slot is used with --all');
> > > +
> > > +# run pg_createsubscriber with '--subscription' and '--all' and
> > > +# verify the failure
> > > +command_fails_like(
> > > + [
> > > + 'pg_createsubscriber',
> > > + '--verbose',
> > > + '--dry-run',
> > > + '--pgdata' => $node_s->data_dir,
> > > + '--publisher-server' => $node_p->connstr($db1),
> > > + '--socketdir' => $node_s->host,
> > > + '--subscriber-port' => $node_s->port,
> > > + '--all',
> > > + '--subscription' => 'sub1',
> > > + ],
> > > + qr/--subscription cannot be used with --all/,
> > > + 'fail if --subscription is used with --all');
> > > +
> > >
> > > Just to cover all combinations we need to test both scenarios. where
> > > --all comes before an incompatible option and vice versa.
> > >
> > > # Run pg_createsubscriber on node S. --verbose is used twice
> > > # to show more information.
> > > # In passing, also test the --enable-two-phase option
> > > @@ -459,10 +526,93 @@ my $sysid_s = $node_s->safe_psql('postgres',
> > > 'SELECT system_identifier FROM pg_control_system()');
> > > ok($sysid_p != $sysid_s, 'system identifier was changed');
> > >
> > > +# On node P create test tables
> > > +$node_p->safe_psql('postgres', 'CREATE TABLE tbl1 (a text)');
> > > +$node_p->safe_psql($db1, 'CREATE TABLE tbl2 (a text)');
> > > +$node_p->safe_psql($db1, "INSERT INTO tbl2 VALUES('first row')");
> > > +$node_p->safe_psql($db2, 'CREATE TABLE tbl3 (a text)');
> > >
> > > $db1 and $db2 already have tables. We can avoid creating new ones here.
> > >
> >
> > Fixed.
> >
> > > +
> > > +# Wait subscriber to catch up
> > > +$node_u->wait_for_subscription_sync($node_p, $subnames1[0]);
> > > +$node_u->wait_for_subscription_sync($node_p, $subnames1[1]);
> > > +$node_u->wait_for_subscription_sync($node_p, $subnames1[2]);
> > > +
> > >
> > > This function just makes sure that initial sync is done. But it
> > > doesn't wait for replication to catch up with current state of the
> > > publisher. It's the latter which would make sure that the last INSERTS
> > > are visible. We should be using wait_for_slot_catchup() on the
> > > publisher.
> > >
> > > +# Check result in database 'postgres' of node U
> > > +$result = $node_u->safe_psql('postgres', 'SELECT * FROM tbl1');
> > > +is($result, qq(first row), "logical replication works in database postgres");
> > > +
> > > +# Check result in database $db1 of node U
> > > +$result = $node_u->safe_psql($db1, 'SELECT * FROM tbl2');
> > > +is( $result, qq(first row
> > > +second row),
> > > + "logical replication works in database $db1");
> > > +
> > > +# Check result in database $db2 of node U
> > > +$result = $node_u->safe_psql($db2, 'SELECT * FROM tbl3');
> > > +is($result, qq(first row), "logical replication works in database $db2");
> > > +
> > >
> > > These tests may not always pass if we use wait_for_slot_catchup above.
> > >
> >
> > During the recent testing, I observed that the tests were failing when
> > using wait_for_slot_catchup(). To address this, I reverted to using
> > wait_for_subscription_sync(), which was employed previously and has
> > proven to be more reliable in ensuring test stability.
> > Please let me know if there are any additional adjustments you would
> > suggest or if you would like me to investigate further into
> > wait_for_slot_catchup().
>
> wait_for_subscription_sync() calls wait_for_catchup() which actually
> waits for a given WAL sender to pass given LSN. So it's working fine.
> But wait_for_subscription_sync() is used to make sure that the initial
> data sync is completed (at least in the cases that I looked at) not
> that the regular replication has caught up. And it's doing more work
> that required. I see other tests using wait_for_catchup() for this
> purpose. Should we use that with appropriate arguments?
>
> --

The v20-0003 patch attached at [1] contains the suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8RjJ4AQCYRanC-9rX9xjZnzH0LWgFyS6Ve-1-gHNG-i5%3DAQ%40mail.gmail.com

Thanks and regards,
Shubham Khanna.