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

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."?

+ 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>
@@ -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."

+# 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.

+
+# 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.

--
Best Wishes,
Ashutosh Bapat