Re: Add support for specifying tables in pg_createsubscriber.

From: Peter Smith <smithpb2250(at)gmail(dot)com>
To: Shubham Khanna <khannashubham1197(at)gmail(dot)com>
Cc: "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Add support for specifying tables in pg_createsubscriber.
Date: 2025-09-16 08:47:14
Message-ID: CAHut+Pu-YhKV=MpJjxG27v6No7x4KUUExoANcZV9bpkejyW86A@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Shubham,

Here are some v8 review comments.

======
Commit message

1.
Previously, pg_createsubscriber would fail if any specified publication already
existed. Now, existing publications are reused as-is with their current
configuration, and non-existing publications are createdcautomatically with
FOR ALL TABLES.

~

typo: "createdcautomatically"

======
doc/src/sgml/ref/pg_createsubscriber.sgml

2.
+ <para>
+ Use <option>--dry-run</option> to see which publications will be reused
+ and which will be created before running the actual command.
+ When publications are reused, they will not be dropped during cleanup
+ operations, ensuring they remain available for other uses.
+ Only publications created by
+ <application>pg_createsubscriber</application> on the target server will
+ be cleaned up if the operation fails. Publications on the publisher
+ server are never modified or dropped by cleanup operations.
+ </para>

I still find all this confusing, for the following reasons:

* The "dry-run" advice is OK, but the "cleanup of existing pubs" seems
like a totally different topic which has nothing much to do with the
--publication option. I'm wondering if you need to talk about cleaning
existing pubs, maybe that info belongs in the "Notes" part of this
documentation.

* I don't understand how does saying "existing pubs will not be
dropped" reconcile with the --clean=publication option which says it
will drop all publications? They seem contradictory.

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

check_and_drop_publications:

3.
/*
- * In dry-run mode, we don't create publications, but we still try to drop
- * those to provide necessary information to the user.
+ * Only drop publications that were created by pg_createsubscriber during
+ * this operation. Pre-existing publications are preserved.
*/
- if (!drop_all_pubs || dry_run)
+ if (dbinfo->made_publication)
drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
&dbinfo->made_publication);

3a.
Sorry, but I still have the same question that I had in my previous v7
review. This function logic will already remove all pre-existing
publications when the 'drop_all_pubs' variable is true. It seems
contrary to the commit message that says "When publications are
reused, they are never dropped during cleanup operations, ensuring
pre-existing publications remain available for other uses."

~

3b.
Kind of similar to the previous review -- If the 'drop_all_pubs'
variable is true, then AFAICT this code attempts to drop again one of
the publications that was already dropped in the earlier part of this
function?

~

3c.
If this drop logic is broken wrt the intended cleanup rules then it
also means more/better --clean option tests are needed, otherwise how
was this passing the tests?

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message jian he 2025-09-16 08:52:02 Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Previous Message Amit Kapila 2025-09-16 08:34:34 Re: Conflict detection for update_deleted in logical replication