On Tue, Apr 4, 2023 at 8:33 PM Jeff Davis <pgsql@j-davis.com> wrote:
> 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."
This sounds great. I think it's very clear about what is being changed
and why. I see that Bertrand already pulled this language into v60.
> 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().
Right, we don't want to go overboard, but I think putting some of the
text you wrote above for the commit message, or something with a
similar theme, in the comment for WalSndWakeup() would be quite
helpful. We want people to understand why the physical and logical
cases are different.
I agree with you that ApplyWalRecord() is the other place where we
need a good comment. I think the one in v60 needs more word-smithing.
It should probably be a bit more detailed and clear about not only
what we're doing but why we're doing it.
The comment in InitWalSenderSlot() seems like it might be slightly
overdone, but I don't have a big problem with it so if we leave it
as-is that's fine.
Now that I understand what's going on here a bit better, I'm inclined
to think that this patch is basically fine. At least, I don't see any
obvious problem with it.
--
Robert Haas
EDB: http://www.enterprisedb.com