On Wednesday, January 31, 2024 9:57 AM Peter Smith <smithpb2...@gmail.com>
wrote:
>
> Hi,
>
> I saw that v73-0001 was pushed, but it included some last-minute
> changes that I did not get a chance to check yesterday.
>
> Here are some review comments for the new parts of that patch.
>
> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 1.
> connect (boolean)
>
> Specifies whether the CREATE SUBSCRIPTION command should connect
> to the publisher at all. The default is true. Setting this to false
> will force the values of create_slot, enabled, copy_data, and failover
> to false. (You cannot combine setting connect to false with setting
> create_slot, enabled, copy_data, or failover to true.)
>
> ~
>
> I don't think the first part "Setting this to false will force the
> values ... failover to false." is strictly correct.
>
> I think is correct to say all those *other* properties (create_slot,
> enabled, copy_data) are forced to false because those otherwise have
> default true values. But the 'failover' has default false, so it
> cannot get force-changed at all because you can't set connect to false
> when failover is true as the second part ("You cannot combine...")
> explains.
>
> IMO remove 'failover' from that first sentence.
>
>
> 3.
> dump can be restored without requiring network access to the remote
> servers. It is then up to the user to reactivate the subscriptions in a
> suitable way. If the involved hosts have changed, the connection
> - information might have to be changed. It might also be appropriate to
> + information might have to be changed. If the subscription needs to
> + be enabled for
> + <link
> linkend="sql-createsubscription-params-with-failover"><literal>failover</lit
> eral></link>,
> + then same needs to be done by executing
> + <link linkend="sql-altersubscription-params-set">
> + <literal>ALTER SUBSCRIPTION ... SET(failover = true)</literal></link>
> + after the slot has been created. It might also be appropriate to
>
> "then same needs to be done" (English?)
>
> BEFORE
> If the subscription needs to be enabled for failover, then same needs
> to be done by executing ALTER SUBSCRIPTION ... SET(failover = true)
> after the slot has been created.
>
> SUGGESTION
> If the subscription needs to be enabled for failover, execute ALTER
> SUBSCRIPTION ... SET(failover = true) after the slot has been created.
>
> ======
> src/backend/commands/subscriptioncmds.c
>
> 4.
> #define SUBOPT_RUN_AS_OWNER 0x00001000
> -#define SUBOPT_LSN 0x00002000
> -#define SUBOPT_ORIGIN 0x00004000
> +#define SUBOPT_FAILOVER 0x00002000
> +#define SUBOPT_LSN 0x00004000
> +#define SUBOPT_ORIGIN 0x00008000
> +
>
> A spurious blank line was added.
> Here is a small patch to address the comment 3 and 4. The discussion for comment 1 is still going on, so we can update the patch once it's concluded. Best Regards, Hou zj
0001-clean-up-for-776621a5.patch
Description: 0001-clean-up-for-776621a5.patch
