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
