On Saturday, January 27, 2024 11:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jan 25, 2024 at 6:42 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > Here is the V69 patch set which includes the following changes.
> >
> > V69-0001, V69-0002
> >
>
> Few minor comments on v69-0001
> 1. In libpqrcv_create_slot(), I see we are using two types of syntaxes based on
> 'use_new_options_syntax' (aka server_version >= 15) whereas this new 'failover'
> option doesn't follow that. What is the reason of the same? I thought it is
> because older versions anyway won't support this option. However, I guess we
> should follow the syntax of the old server and let it error out.
Changed as suggested.
> BTW, did you test
> this patch with old server versions (say < 15 and >=15) by directly using
> replication commands, if so, what is the behavior of same?
Yes, I tested it. We cannot use new failover option or new
alter_replication_slot on server <17, the errors we will get are as follows:
Using failover option in create_replication_slot on server 15 ~ 16
ERROR: unrecognized option: failover
Using failover option in create_replication_slot on server < 15
ERROR: syntax error
Alter_replication_slot on server < 17
ERROR: syntax error at or near "ALTER_REPLICATION_SLOT"
>
> 2.
> }
> -
> + if (failover)
> + appendStringInfoString(&cmd, "FAILOVER, ");
>
> Spurious line removal. Also, to follow a coding pattern similar to nearby code,
> let's have one empty line after handling of failover.
Changed.
>
> 3.
> +/* ALTER_REPLICATION_SLOT slot */
> +alter_replication_slot:
> + K_ALTER_REPLICATION_SLOT IDENT '(' generic_option_list ')'
>
> I think it would be better if we follow the create style by specifying syntax in
> comments as that can make the code easier to understand after future
> extensions to this command if any. See
> create_replication_slot:
> /* CREATE_REPLICATION_SLOT slot [TEMPORARY] PHYSICAL [options] */
> K_CREATE_REPLICATION_SLOT IDENT opt_temporary K_PHYSICAL
> create_slot_options
Changed.
Attach the V70 patch set which addressed above comments and Bertrand's comments in [1]
[1] https://www.postgresql.org/message-id/ZbNt1oRZRcdIAw2c%40ip-10-97-1-34.eu-west-3.compute.internal
Best Regards,
Hou zj