On Tue, 12 Sept 2023 at 14:25, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> 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?
Yes, there is a possibility that the apply worker gets started and new
transaction data is being synced from the publisher. I have made a fix
not to start the launcher process in binary ugprade mode as we don't
want the launcher to start apply worker during upgrade.
> 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.
Modified
> 03. Parameter combinations
>
> IIUC getSubscriptionTables() should be exitted quickly if --no-subscriptions is
> specified, whereas binary_upgrade_create_sub_rel_state() is failed.
Modified
>
> 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.
Modified
The attached v8 version patch has the changes for the same.
Regards,
Vignesh