RE: Time delayed LR (WAS Re: logical replication restrictions)

Dear Amit,

> Few comments:

Thank you for reviewing! PSA new version.
Note that the starting point of delay for 2PC was not changed,
I think it has been under discussion.

> 1.
> + /*
> + * If we've requested to shut down, exit the process.
> + *
> + * Note that WalSndDone() cannot be used here because the delaying
> + * changes will be sent in the function.
> + */
> + if (got_STOPPING)
> + {
> + QueryCompletion qc;
> +
> + /* Inform the standby that XLOG streaming is done */
> + SetQueryCompletion(&qc, CMDTAG_COPY, 0);
> + EndCommand(&qc, DestRemote, false);
> + pq_flush();
>
> Do we really need to do anything except for breaking the loop and let
> the exit handling happen in the main loop when 'got_STOPPING' is set?
> AFAICS, this is what we are doing in some other palces (See
> WalSndWaitForWal). Won't that work? It seems that will help us sending
> all the pending WAL.

If we exit the loop after got_STOPPING is set, as you said, the walsender will
send delaying changes and then exit. The behavior is same as the case that WalSndDone()
is called. But I think it is not suitable for the motivation of the feature.
If users notice the miss operation like TRUNCATE, they must shut down the publisher
once and then recovery from back up or old subscriber. If the walsender sends all
pending changes, miss operations will be also propagated to subscriber and data
cannot be protected. So currently I want to keep the style.
FYI - In case of physical replication, received WALs are not applied when the
secondary is shutted down.

> 2.
> + /* Try to flush pending output to the client */
> + if (pq_flush_if_writable() != 0)
> + WalSndShutdown();
>
> Is there a reason to try flushing here?

IIUC if pq_flush_if_writable() returns non-zero (EOF), it means that there is a
trouble and walsender fails to send messages to subscriber.

In Linux, the stuck trace from pq_flush_if_writable() will finally reach the send() system call.
And according to man page[1], it will be triggered by some unexpected state or the connection is closed.

Based on above, I think the returned value should be confirmed.

> Apart from the above, I have made a few changes in the comments in the
> attached diff patch. If you agree with those then please include them
> in the next version.

Thanks! I checked and I thought all of them should be included.

Moreover, I used grammar checker and slightly reworded the commit message.

[1]: https://man7.org/linux/man-pages/man3/send.3p.html

Best Regards,
Hayato Kuroda
FUJITSU LIMITED