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: 'Masahiko Sawada' <sawada.mshk@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, "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-09T10:12:03Z
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.

Dear Sawada-san,

Thank you for reviewing!

> +/*
> + * Options for controlling the behavior of the walsender. Options can be
> + * specified in the START_STREAMING replication command. Currently only one
> + * option is allowed.
> + */
> +typedef struct
> +{
> +        WalSndShutdownMode shutdown_mode;
> +} WalSndOptions;
> +
> +static WalSndOptions *my_options = NULL;
> 
> I'm not sure we need to have it as a struct at this stage since we
> support only one option. I wonder if we can have one value, say
> shutdown_mode, and we can make it a struct when we really need it.
> Even if we use WalSndOptions struct, I don't think we need to
> dynamically allocate it. Since a walsender can start logical
> replication multiple times in principle, my_options is not freed.

+1, removed the structure.

> ---
> +/*
> + * Parse given shutdown mode.
> + *
> + * Currently two values are accepted - "wait_flush" and "immediate"
> + */
> +static void
> +ParseShutdownMode(char *shutdownmode)
> +{
> +        if (pg_strcasecmp(shutdownmode, "wait_flush") == 0)
> +                my_options->shutdown_mode =
> WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
> +        else if (pg_strcasecmp(shutdownmode, "immediate") == 0)
> +                my_options->shutdown_mode =
> WALSND_SHUTDOWN_MODE_IMMIDEATE;
> +        else
> +                ereport(ERROR,
> +                                errcode(ERRCODE_SYNTAX_ERROR),
> +                                errmsg("SHUTDOWN_MODE requires
> \"wait_flush\" or \"immediate\""));
> +}
> 
> I think we should make the error message consistent with other enum
> parameters. How about the message like:
> 
> ERROR:  invalid value shutdown mode: "%s"

Modified like enum parameters and hint message was also provided.

New patch is attached in [1].

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

Best Regards,
Hayato Kuroda
FUJITSU LIMITED