Re: Add support for specifying tables in pg_createsubscriber. - Mailing list pgsql-hackers

On Thu, 6 Nov 2025 at 09:35, Shubham Khanna <khannashubham1197@gmail.com> wrote:
>
> On Thu, Nov 6, 2025 at 7:18 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Wed, Nov 5, 2025 at 3:42 PM Shubham Khanna
> > <khannashubham1197@gmail.com> wrote:
> > >
> > > On Mon, Nov 3, 2025 at 12:55 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > > > Hi Shubham.
> > > >
> > > > A comment about the v17-0001.
> > > >
> > > > ======
> > > > 1.
> > > > + if (check_publication_exists(conn, dbinfo[i].pubname, dbinfo[i].dbname))
> > > > + {
> > > > + /* Reuse existing publication on publisher. */
> > > > + pg_log_info("dry-run: would use existing publication \"%s\" in
> > > > database \"%s\"",
> > > > + dbinfo[i].pubname, dbinfo[i].dbname);
> > > > + dbinfo[i].made_publication = false;
> > > > + }
> > > >
> > > > Is that correct? Won't this code now unconditionally log with the
> > > > "dry-run:" prefix, even when the tool is *not* doing a dry-run?
> > > >
> > > > I thought code would be something like:
> > > >
> > > > SUGGESTION #1 (if/else)
> > > > /* Reuse existing publication on publisher. */
> > > > if (dry_run)
> > > >   pg_log_info("dry-run: would use existing publication ...);
> > > > else
> > > >   pg_log_info("use existing publication ...);
> > > >
> > > > ~~~
> > > >
> > > > OTOH, (since here is just an info message with no destructive
> > > > operation) perhaps it would be harmless also to keep the original log
> > > > message for both dry-run and normal mode.
> > > >
> > > > SUGGESTION #2 (do nothing)
> > > > pg_log_info("use existing publication ...);
> > > >
> > > > ======
> > >
> > > Hi Peter,
> > >
> > > Thank you for your review and suggestions.
> > > I agree with your reasoning regarding the logging behavior. I will
> > > proceed with Suggestion #2 and retain the existing `pg_log_info("use
> > > existing publication ...");` message for both dry-run and normal
> > > modes. This message is purely informational and does not perform any
> > > destructive action, making it suitable for both scenarios.
> > > The attached patch includes these changes.
> > >
> >
> > OK, but you did not do quite what you said you did.
> >
> > Notice that v18 has modified the message to "would use existing
> > publication...", instead of just leaving it how it was in v16 ("use
> > existing publication...").
> >
> > ======
>
> Fixed.
>
> The attached patch includes these changes.

Few comments:
1) The test seems to be incomplete as the result is not verified after
the select statement:
+# Get subscription names and publications
+$result = $node_s2->safe_psql(
+       'postgres', qq(
+    SELECT subname, subpublications FROM pg_subscription WHERE
subname ~ '^pg_createsubscriber_'
+));
+@subnames = split("\n", $result);
+
+# Check result in database $db1
+$result = $node_s2->safe_psql($db1, 'SELECT * FROM tbl1');

2) Here subnames is not used anywhere, is it required?
+@subnames = split("\n", $result);
+
+# Check result in database $db1
+$result = $node_s2->safe_psql($db1, 'SELECT * FROM tbl1');

3) Since select is on a single table, no need to use alias s, and the
column name can be used directly i.e. subpublications instead of
s.subpublications:
+# Verify that the correct publications are being used
+$result = $node_s2->safe_psql(
+       'postgres', qq(
+        SELECT s.subpublications
+        FROM pg_subscription s
+        WHERE s.subname ~ '^pg_createsubscriber_'
+        ORDER BY s.subdbid
+    )
+);

4) Let's include the database name too. How about something like
SELECT subdbid::regdatabase, subpublications ...

5) Since both the verification are same but only the db is different,
we can change it to keep messages similar:
+# Verify dry-run did not modify publisher state
+my $pub_names_db1 = $node_p->safe_psql($db1,
+       "SELECT pubname FROM pg_publication ORDER BY pubname");
+is( $pub_names_db1, qq(pub1
+test_pub1
+test_pub2
+test_pub_existing),
+       "existing publication remains unchanged after dry-run");
+
+my $pub_names_db2 = $node_p->safe_psql($db2,
+       "SELECT pubname FROM pg_publication ORDER BY pubname");
+is($pub_names_db2, 'pub2',
+       "dry-run did not actually create publications in db2");

6) No need for new variables pub_names_db1 and pub_names_db2 here, we
can use existing result variable instead.

Regards,
Vignesh