Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. - Mailing list pgsql-hackers

On Fri, Mar 21, 2025 at 6:59 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> > <khannashubham1197@gmail.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?

--
Best Wishes,
Ashutosh Bapat