Re: Add support for specifying tables in pg_createsubscriber.

Hi Shubham,

Here are some v10 review comments.

======
1. GENERAL

A couple of my review comments below (#4, #7) are about the tense of
the info messages; My comments are trying to introduce some
consistency in the patch but unfortunately I see there are already
lots of existing messages where the tense is a muddle of past/present
(e.g. "creating"/"could not create" instead of "created"/"could not
create" or "creating"/"cannot create". Perhaps it's better to just
ignore my pg_log_info comments and do whatever seems right for this
patch, but also make another thread to fix the tense for all the
messages in one go.

======
Commit Message

2.
When publications are reused, they are never dropped during cleanup operations,
ensuring pre-existing publications remain available for other uses.
Only publications created by pg_createsubscriber are cleaned up.

~

AFAICT, the implemented behaviour has flip-flopped again regarding
--clean; I think now --clean=publications is unchanged from master
(e.g. --clean=publication will drop all publications).

That's OK, but you need to ensure the commit message is saying the
same thing. e.g. currently it says "cleanup operations" never drop
reused publications, but what about "--clean=publications" -- that's a
cleanup operation, isn't it?

======
src/bin/pg_basebackup/pg_createsubscriber.c

check_publication_exists:

3.
+/*
+ * Check if a publication with the given name exists in the specified database.
+ * Returns true if it exists, false otherwise.
+ */

It seems a verbose way of saying:
Return whether a specified publication exists in the specified database.

~~~

check_and_drop_publications:

4.
+ if (check_publication_exists(conn, dbinfo[i].pubname, dbinfo[i].dbname))
+ {
+ /* Reuse existing publication on publisher. */
+ pg_log_info("using existing publication \"%s\" in database \"%s\"",
+ dbinfo[i].pubname, dbinfo[i].dbname);
+ dbinfo[i].made_publication = false;
+ }
+ else
+ {
+ /*
+ * Create publication on publisher. This step should be executed
+ * *before* promoting the subscriber to avoid any transactions
+ * between consistent LSN and the new publication rows (such
+ * transactions wouldn't see the new publication rows resulting in
+ * an error).
+ */
+ create_publication(conn, &dbinfo[i]);
+ pg_log_info("created publication \"%s\" in database \"%s\"",
+ dbinfo[i].pubname, dbinfo[i].dbname);
+ dbinfo[i].made_publication = true;
+ }

The tense of these messages seems inconsistent, and is also different
from another nearby message: "create replication slot..."

So, should these pg_log_info change to match? For example:
"using existing publication" --> "use existing..."
"created publication" --> "create publication..."

~~~

check_and_drop_publications:

5.
* Since the publications were created before the consistent LSN, they
* remain on the subscriber even after the physical replica is
* promoted. Remove these publications from the subscriber because
- * they have no use. Additionally, if requested, drop all pre-existing
- * publications.
+ * they have no use. If --clean=publications is specified, drop all existing
+ * publications in the database. Otherwise, only drop publications that were
+ * created by pg_createsubscriber during this operation, preserving any
+ * pre-existing publications.
*/

This function comment seems overkill now that this function has
evolved. e.g. That whole first part (before "If --clean...") seems
like it would be better just as a comment within the function body
'else' block rather than needing to say anything in the function
comment.

~~~

6.
for (int i = 0; i < PQntuples(res); i++)
drop_publication(conn, PQgetvalue(res, i, 0), dbinfo->dbname,
&dbinfo->made_publication);
-
PQclear(res);
This whitespace change is not needed for the patch.

~~~

7.
+ else
+ {
+ /*
+ * Only drop publications that were created by pg_createsubscriber
+ * during this operation. Pre-existing publications are preserved.
+ */
+ if (dbinfo->made_publication)
+ drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
+ &dbinfo->made_publication);
+ else
+ pg_log_info("preserving existing publication \"%s\" in database \"%s\"",
+ dbinfo->pubname, dbinfo->dbname);
+ }

For consistency with earlier review comment, maybe the message should
be reworded:
"preserving existing publication" --> "preserve existing publication"
======
.../t/040_pg_createsubscriber.pl

8.
+# Run pg_createsubscriber on node S2 without --dry-run
+command_ok(

and later...

+# Run pg_createsubscriber on node S2 without --dry-run
+command_ok(

~

These are not very informative comments. It should say more about the
purpose -- e.g. you are specifying --publication to reuse one
publication and create another ... to demonstrate that yada yada...

~~~

9.
+# Verify the existing publication is still there and unchanged
+my $existing_pub_count = $node_p->safe_psql($db1,
+ "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'test_pub_existing'"
+);
+is($existing_pub_count, '1',
+ 'existing publication remains unchanged after dry-run');
+
+# Verify no actual publications were created during dry-run
+my $pub_count_after_dry_run = $node_p->safe_psql($db2,
+ "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'test_pub_new'");
+is($pub_count_after_dry_run, '0',
+ 'dry-run did not actually create publications');
+

Instead of 2 SQLs to check whether something exists and something else
does not exist, can't you just have 1 SELECT to fetch all publication
names, then you can deduce the same thing from the result.

~~~

10.
+# Run pg_createsubscriber on node S3 with --dry-run
+($stdout, $stderr) = run_command(

Again, it's not a very informative comment. It should say more about
the purpose -- e.g. you are specifying --publication and
--clean=publications at the same time ... to demonstrate yada yada...

~~~

11.
+# Verify nothing was actually changed
+my $existing_pub_still_exists = $node_p->safe_psql($db1,
+ "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'test_pub_existing'"
+);
+is($existing_pub_still_exists, '1',
+ 'existing publication still exists after dry-run with --clean');
+
+my $new_pub_still_exists = $node_p->safe_psql($db2,
+ "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'test_pub_new'");
+is($new_pub_still_exists, '1',
+ 'pg_createsubscriber publication still exists after dry-run with --clean'
+);
+

Isn't this another example of something that is easily verified with a
single SQL instead of checking both publications separately?

~~~

12.
# Run pg_createsubscriber on node S3 with --clean option to verify that the
# existing publications are preserved.
command_ok(

Is that comment correct? I don't think so because --clean=publications
is supposed to drop all publications, right?

~~~

13.
+# Confirm ALL publications were removed (both existing and new)
+is( $node_s3->safe_psql(
+ $db1,
+ "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'test_pub_existing';"
+ ),
+ '0',
+ 'pre-existing publication was removed by --clean=publications');
+
+is( $node_s3->safe_psql(
+ $db2,
+ "SELECT COUNT(*) FROM pg_publication WHERE pubname = 'test_pub_new';"
+ ),
+ '0',
+ 'publication created by pg_createsubscriber was removed by
--clean=publications'
+);
+

Are 2x SELECT needed here? Can't you just have a single select to
discover that there are zero publications?

======
Kind Regards,
Peter Smith
Fujitsu Australia