RE: [PoC] pg_upgrade: allow to upgrade publisher node

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>

To: "Wei Wang (Fujitsu)" <wangw.fnst@fujitsu.com>, 'Peter Smith' <smithpb2250@gmail.com>

Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Julien Rouhaud <rjuju123@gmail.com>, vignesh C <vignesh21@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>

Date: 2023-05-11T08:55:08Z

Lists: pgsql-hackers

Attachments

v12-0001-pg_upgrade-Add-include-logical-replication-slots.patch (application/octet-stream) patch v12-0001
v12-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch (application/octet-stream) patch v12-0002
v12-0003-pg_upgrade-Add-check-function-for-include-logica.patch (application/octet-stream) patch v12-0003
v12-0004-Change-the-method-used-to-check-logical-replicat.patch (application/octet-stream) patch v12-0004

Dear Wang,

Thank you for reviewing! PSA new version.

> 1. In the function getLogicalReplicationSlots
> ```
> +		/*
> +		 * Note: Currently we do not have any options to include/exclude
> slots
> +		 * in dumping, so all the slots must be selected.
> +		 */
> +		slotinfo[i].dobj.dump = DUMP_COMPONENT_ALL;
> ```
> I think currently we are only dumping the definition of logical replication
> slots. It seems better to set it as DUMP_COMPONENT_DEFINITION here.

Right. Actually it was harmless because another flags like DUMP_COMPONENT_DEFINITION
are not checked in dumpLogicalReplicationSlot(), but changed.

> 2. In the function dumpLogicalReplicationSlot
> ```
> +		ArchiveEntry(fout, slotinfo->dobj.catId, slotinfo->dobj.dumpId,
> +					 ARCHIVE_OPTS(.tag = slotname,
> +
> 	  .description = "REPLICATION SLOT",
> +								  .section =
> SECTION_POST_DATA,
> +
> 	  .createStmt = query->data));
> ```
> I think if we do not set the member dropStmt in macro ARCHIVE_OPTS here, when
> we
> specifying the option "--logical-replication-slots-only" and option "-c/--clean"
> together, the "-c/--clean" will not work.
> 
> I think that we could use the function pg_drop_replication_slot to set this
> member. Then, in the main function in the pg_dump.c file, we should add a check
> to prevent specifying option "--logical-replication-slots-only" and
> option "--if-exists" together.
> Or, we could simply add a check to prevent specifying option
> "--logical-replication-slots-only" and option "-c/--clean" together.
> What do you think?

I chose not to allow to combine with -c. Assuming that this option is used only
by the pg_upgrade, it is ensured that new node does not have any logical replication
slots. So the remove function is not needed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Commits

Fix issues in binary_upgrade_logical_slot_has_caught_up().
- 0bf62460bb9e 17.0 landed
Fix a random failure in 003_logical_slots.pl.
- 63c5df126abb 17.0 landed
Fix a test in 003_logical_slots.
- 3e36e48d8efe 17.0 landed
Fix uninitialized slot array access during the upgrade.
- a7db71ed2787 17.0 landed
Fix the test 003_logical_slots.
- 8af917be6bad 17.0 landed
Commit b195e6d482 forgot to update meson.build.
- 8949b978ff49 17.0 landed
Use shorter file names in the upgrade logical slots test.
- b195e6d482b8 17.0 landed
Migrate logical slots to the new node during an upgrade.
- 29d0a77fa660 17.0 landed
Flush logical slots to disk during a shutdown checkpoint if required.
- e0b2eed047df 17.0 landed
Prevent possibility of panics during shutdown checkpoint.
- c6c333436491 10.0 cited