Re: Exit walsender before confirming remote flush in logical replication

Hi,

On Wed, Feb 8, 2023 at 6:47 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> >
> > Dear Horiguchi-san,
> >
> > Thank you for checking the patch! PSA new version.
>
> PSA rebased patch that supports updated time-delayed patch[1].
>

Thank you for the patch! Here are some comments on v5 patch:

+/*
+ * 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.

---
+/*
+ * 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"

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com