Horton

RE: Exit walsender before confirming remote flush in logical replication

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: 'Kyotaro Horiguchi' <horikyota.ntt@gmail.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, "sawada.mshk@gmail.com" <sawada.mshk@gmail.com>, "michael@paquier.xyz" <michael@paquier.xyz>, "peter.eisentraut@enterprisedb.com" <peter.eisentraut@enterprisedb.com>, "dilipbalaut@gmail.com" <dilipbalaut@gmail.com>, "andres@anarazel.de" <andres@anarazel.de>, "amit.kapila16@gmail.com" <amit.kapila16@gmail.com>
Date: 2023-02-08T08:01:24Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add wal_sender_shutdown_timeout GUC to limit shutdown wait for replication

  2. pg_upgrade: Add --copy option

  3. Prevent possibility of panics during shutdown checkpoint.

  4. Support clean switchover.

Attachments

Dear Horiguchi-san,

Thank you for checking the patch! PSA new version.

> 0002:
> 
> This patch doesn't seem to offer a means to change the default
> walsender behavior.  We need a subscription option named like
> "walsender_exit_mode" to do that.

As I said in another mail[1], I'm thinking the feature does not have to be
used alone for now.

> +ConsumeWalsenderOptions(List *options, WalSndData *data)
> 
> I wonder if it is the right design to put options for different things
> into a single list. I rather choose to embed the walsender option in
> the syntax than needing this function.
> 
> K_START_REPLICATION opt_slot opt_physical RECPTR opt_timeline
> opt_shutdown_mode
> 
> K_START_REPLICATION K_SLOTIDENT K_LOGICAL RECPTR
> opt_shutdown_mode plugin_options
> 
> where opt_shutdown_mode would be like "SHUTDOWN_MODE immediate".

Right, the option handling was quite bad. I added new syntax opt_shutdown_mode
to logical replication. And many codes were modified accordingly.
Note that based on the other discussion, I removed codes
for supporting physical replication but tried to keep the extensibility.

> ======
> If we go with the current design, I think it is better to share the
> option list rule between the logical and physical START_REPLCIATION
> commands.
> 
> I'm not sure I like the option syntax
> "exit_before_confirming=<Boolean>". I imagin that other options may
> come in future. Thus, how about "walsender_shutdown_mode=<mode>",
> where the mode is one of "wait_flush"(default) and "immediate"?

Seems better, I changed to from boolean to enumeration.

> +typedef struct
> +{
> +	bool		exit_before_confirming;
> +} WalSndData;
> 
> Data doesn't seem to represent the variable. Why not WalSndOptions?

This is inspired by PGOutputData, but I prefer your idea. Fixed.

> -		!equal(newsub->publications, MySubscription->publications))
> +		!equal(newsub->publications, MySubscription->publications) ||
> +		(newsub->minapplydelay > 0 &&
> MySubscription->minapplydelay == 0) ||
> +		(newsub->minapplydelay == 0 &&
> MySubscription->minapplydelay > 0))
> 
>  I slightly prefer the following expression (Others may disagree:p):
> 
>   ((newsub->minapplydelay == 0) != (MySubscription->minapplydelay == 0))

I think conditions for the same parameter should be aligned one line,
So your posted seems better. Fixed.

> 
>  And I think we need a comment for the term. For example,
> 
>   /* minapplydelay affects START_REPLICATION option exit_before_confirming
> */

Added just above the condition.

> + * Reads all entrly of the list and consume if needed.
> s/entrly/entries/ ?
> ...

This part is no longer needed.

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866D3EC780D251953BDE7FAF5D89%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED