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
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 |