Horton

Re: Exit walsender before confirming remote flush in logical replication

Kyotaro Horiguchi <horikyota.ntt@gmail.com>

View thread
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: kuroda.hayato@fujitsu.com
Cc: pgsql-hackers@postgresql.org, sawada.mshk@gmail.com, michael@paquier.xyz, peter.eisentraut@enterprisedb.com, dilipbalaut@gmail.com, andres@anarazel.de, amit.kapila16@gmail.com
Date: 2023-02-08T02:27:17Z
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.

I agree to the direction and thanks for the patch.

At Tue, 7 Feb 2023 17:08:54 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in 
> > I noticed that previous ones are rejected by cfbot, even if they passed on my
> > environment...
> > PSA fixed version.
> 
> While analyzing more, I found the further bug that forgets initialization.
> PSA new version that could be passed automated tests on my github repository.
> Sorry for noise.

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.


+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".


======
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"?


+typedef struct
+{
+	bool		exit_before_confirming;
+} WalSndData;

Data doesn't seem to represent the variable. Why not WalSndOptions?


-		!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))

 And I think we need a comment for the term. For example,

  /* minapplydelay affects START_REPLICATION option exit_before_confirming */


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

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center