Re: Add support for specifying tables in pg_createsubscriber.

On Fri, Nov 28, 2025 at 11:36 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 6 Nov 2025 at 09:35, Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Thu, Nov 6, 2025 at 7:18 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Nov 5, 2025 at 3:42 PM Shubham Khanna
> > > <khannashubham1197(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Nov 3, 2025 at 12:55 PM Peter Smith <smithpb2250(at)gmail(dot)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_'
> +));
> +(at)subnames = split("\n", $result);
> +
> +# Check result in database $db1
> +$result = $node_s2->safe_psql($db1, 'SELECT * FROM tbl1');
>

Fixed.

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

Fixed.

> 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
> + )
> +);
>

Fixed.

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

Fixed.

> 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");
>

Fixed.

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

Fixed.

The attached patch includes these changes.

Thanks and regards,
Shubham Khanna.