Hi, Thankyou for taking the time to look at this and reply. > > I did look at this, and while the explanation in the current comment may > seem a bit confusing, I'm not sure the suggested changes improve the > situation very much. > > This suggests the two comments somehow disagree, but it does not say in > what exactly, so perhaps I just missed it :-( > > ISTM there's a bit of confusion what is meant by "stopping" state - you > seem to be interpreting it as a general concept, where the walsender is > requested to stop (through the signal), and starts doing stuff to exit. > But the comments actually talk about WalSnd->state, where "stopping" > means it needs to be set to WALSNDSTATE_STOPPING.
Yes, I interpreted the "stopping" state meaning as when the boolean flag 'got_STOPPING' is assigned true. > > And we only ever switch to that state in two places - in WalSndPhysical > and exec_replication_command. And that does not happen in regular > logical replication (which is what "logical replication is in progress" > refers to) - if you have a walsender just replicating DML, it will never > see the WALSNDSTATE_STOPPING state. It will simply do the cleanup while > still in WALSNDSTATE_STREAMING state, and then just exit. > > So from this point of view, the suggestion is actually wrong. OK. > > To conclude, I think this probably makes the comments more confusing. If > we want to make it clearer, I'd probably start by clarifying what the > "stopping" state means. Also, it's a bit surprising we may not actually > go through the "stopping" state during shutdown. > I agree. My interpretation of the (ambiguous) "stopping" state led me to believe the comment was quite wrong. So, this thread was only intended as a trivial comment fix in passing but clearly there is more to this than I anticipated. I would be happy if someone with more knowledge about the WALSNDSTATE_STOPPING versus got_STOPPING could disambiguate the file header comment, but that's not me, so I have withdrawn this from the Commitfest. ====== Kind Regards, Peter Smith. Fujitsu Australia
