On Fri, Nov 28, 2025 at 11:36 AM vignesh C <[email protected]> wrote: > > On Thu, 6 Nov 2025 at 09:35, Shubham Khanna <[email protected]> > wrote: > > > > On Thu, Nov 6, 2025 at 7:18 AM Peter Smith <[email protected]> wrote: > > > > > > On Wed, Nov 5, 2025 at 3:42 PM Shubham Khanna > > > <[email protected]> wrote: > > > > > > > > On Mon, Nov 3, 2025 at 12:55 PM Peter Smith <[email protected]> > > > > 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'); >
Fixed.
> 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');
>
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.
v20-0001-Support-existing-publications-in-pg_createsubscr.patch
Description: Binary data
