Re: Minimal logical decoding on standbys

On Tue, 2023-04-04 at 14:55 -0400, Robert Haas wrote:
> Thanks for your continued work on $SUBJECT. I just took a look at
> 0004, and I think that at the very least the commit message needs
> work. Nobody who is not a hacker is going to understand what problem
> this is fixing, because it makes reference to the names of functions
> and structure members rather than user-visible behavior. In fact, I'm
> not really sure that I understand the problem myself. It seems like
> the problem is that on a standby, WAL senders will get woken up too
> early, before we have any WAL to send.

Logical walsenders on the standby, specifically, which didn't exist
before this patch series.

> That's presumably OK, in the
> sense that they'll go back to sleep and eventually wake up again, but
> it means they might end up chronically behind sending out WAL to
> cascading standbys.

Without 0004, cascading logical walsenders would have worse wakeup
behavior than logical walsenders on the primary. Assuming the fix is
small in scope and otherwise acceptable, I think it belongs as a part
of this overall series.

> If that's right, I think it should be spelled out
> more clearly in the commit message, and maybe also in the code
> comments.

Perhaps a commit message like:

"For cascading replication, wake up physical walsenders separately from
logical walsenders.

Physical walsenders can't send data until it's been flushed; logical
walsenders can't decode and send data until it's been applied. On the
standby, the WAL is flushed first, which will only wake up physical
walsenders; and then applied, which will only wake up logical
walsenders.

Previously, all walsenders were awakened when the WAL was flushed. That
was fine for logical walsenders on the primary; but on the standby the
flushed WAL would not have been applied yet, so logical walsenders were
awakened too early."

(I'm not sure if I quite got the verb tenses right.)

For comments, I agree that WalSndWakeup() clearly needs a comment
update. The call site in ApplyWalRecord() could also use a comment. You
could add a comment at every call site, but I don't think that's
necessary if there's a good comment over WalSndWakeup().

Regards,
Jeff Davis