RE: Synchronizing slots from primary to standby - Mailing list pgsql-hackers

From Zhijie Hou (Fujitsu)
Subject RE: Synchronizing slots from primary to standby
Date
Msg-id OS0PR01MB5716DE2A364CB23F9144AED194782@OS0PR01MB5716.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: Synchronizing slots from primary to standby  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: Synchronizing slots from primary to standby
Re: Synchronizing slots from primary to standby
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: vignesh C
Date:
Subject: Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting
Next
From: Alexander Lakhin
Date:
Subject: Re: cleanup patches for incremental backup