Dear Vignesh, Thank you for updating the patch! Here are some comments.
Sorry if there are duplicate comments - the thread revived recently so I might
lose my memory.
01. General
Is there a possibility that apply worker on old cluster connects to the
publisher during the upgrade? Regarding the pg_upgrade on publisher, the we
refuse TCP/IP connections from remotes and port number is also changed, so we
can
assume that subscriber does not connect to. But IIUC such settings may not
affect
to the connection source, so that the apply worker may try to connect to the
publisher. Also, is there any hazards if it happens?
02. Upgrade functions
Two functions - binary_upgrade_create_sub_rel_state and
binary_upgrade_sub_replication_origin_advance
should be located at pg_upgrade_support.c. Also, CHECK_IS_BINARY_UPGRADE() macro
can be used.
03. Parameter combinations
IIUC getSubscriptionTables() should be exitted quickly if --no-subscriptions is
specified, whereas binary_upgrade_create_sub_rel_state() is failed.
04. I failed my test
I executed attached script but failed to upgrade:
```
Restoring database schemas in the new cluster
postgres
*failure*
Consult the last few lines of
"data_N3/pg_upgrade_output.d/20230912T054546.320/log/pg_upgrade_dump_5.log" for
the probable cause of the failure.
Failure, exiting
```
I checked the log and found that binary_upgrade_create_sub_rel_state() does not
support skipping the fourth argument:
```
pg_restore: from TOC entry 4059; 16384 16387 SUBSCRIPTION TABLE sub sub postgres
pg_restore: error: could not execute query: ERROR: function
binary_upgrade_create_sub_rel_state(unknown, integer, unknown) does not exist
LINE 1: SELECT binary_upgrade_create_sub_rel_state('sub', 16384, 'r'...
^
HINT: No function matches the given name and argument types. You might need to
add explicit type casts.
Command was: SELECT binary_upgrade_create_sub_rel_state('sub', 16384, 'r');
```
IIUC if we allow to skip arguments, we must define wrappers like
pg_copy_logical_replication_slot_*.
Another approach is that pg_dump always dumps srsublsn even if it is NULL.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
test.sh
Description: test.sh
