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
