Subject: RE: Synchronizing slots from primary to standby

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 27 Nov 2023 12:23:20 +0000

On Monday, November 27, 2023 8:05 PM Drouvot, Bertrand <[email-masked]> wrote:

Hi,

> On 11/6/23 2:30 AM, Zhijie Hou (Fujitsu) wrote:
> > On Friday, November 3, 2023 7:32 PM Amit Kapila
> <[email-masked]>
> >>
> >> I don't see a corresponding change in repl_gram.y. I think the following part
> of
> >> the code needs to be changed:
> >> /* CREATE_REPLICATION_SLOT slot [TEMPORARY] LOGICAL plugin [options]
> */
> >> | K_CREATE_REPLICATION_SLOT IDENT opt_temporary K_LOGICAL IDENT
> >> create_slot_options
> >>
> >
> > I think after 0266e98, we started to use the new syntax(see the
> > generic_option_list rule) and we can avoid changing the repl_gram.y when
> adding
> > new options. The new failover can be detected when parsing the generic
> option
> > list(in parseCreateReplSlotOptions).
> 
> Did not look in details but it looks like there is more to do here as
> this is failing (with v39_2):
> 
> "
> postgres@primary: psql replication=database
> psql (17devel)
> Type "help" for help.
> 
> postgres=# CREATE_REPLICATION_SLOT test_logical20 LOGICAL pgoutput
> FAILOVER;
> ERROR:  syntax error

I think the command you executed is of old syntax style, which was kept for
compatibility with older releases. And I think we can avoid supporting new
option for the old syntax as described in the original thread[1] of commit
0266e98. So, the "syntax error" is as expected IIUC.

The new style command is like:
CREATE_REPLICATION_SLOT test_logical20 LOGICAL pgoutput (FAILOVER);

[1] https://www.postgresql.org/message-id/CA%2BTgmobAczXDRO_Gr2euo_TxgzaH1JxbNxvFx%3DHYvBinefNH8Q%40mail.gmail.com

Best Regards,
Hou zj

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 27 Nov 2023 13:46:05 +0100

Hi,

> [1] https://www.postgresql.org/message-id/CA%2BTgmobAczXDRO_Gr2euo_TxgzaH1JxbNxvFx%3DHYvBinefNH8Q%40mail.gmail.com

Oh, I see, thanks for pointing out.

Well, not related to that thread but it seems weird to me that the backward
compatibility is done at the "option" level then.

I think it would make more sense to support all the options if the old
syntax is still supported.

For example, having

postgres=# CREATE_REPLICATION_SLOT test_logical2 LOGICAL pgoutput TWO_PHASE;

working fine but

CREATE_REPLICATION_SLOT test_logical3 LOGICAL pgoutput FAILOVER;

failing looks weird to me.

But that's probably out of this thread's context anyway.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 27 Nov 2023 15:08:18 +0100

Hi,

> CREATE_REPLICATION_SLOT test_logical20 LOGICAL pgoutput (FAILOVER);

If / As we are not going to support the old syntax for the FAILOVER option
so I think we can get rid of the check on "use_new_options_syntax" here:

-
+               if (failover)
+               {
+                       appendStringInfoString(&cmd, "FAILOVER");
+                       if (use_new_options_syntax)
+                               appendStringInfoString(&cmd, ", ");
+                       else
+                               appendStringInfoChar(&cmd, ' ');
+               }

as we'd error out before if using the old syntax.

Regards,