On Tue, Apr 4, 2023 at 8:33 PM Jeff Davis <pg...@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
