Re: [PATCH] Clear walsender reader pointers after cleanup

Hi,

On Tue, 13 Oct 2026 at 04:35, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Sun, Oct 11, 2026 at 02:54:27AM -0700, Ayush Tiwari wrote:
> > FreeDecodingContext() frees the reader when logical streaming ends, but
> > xlogreader is left pointing to it. If the next command on that connection
> > fails, WalSndErrorCleanup() still follows that pointer.
>
> You have been able to reach that without patching the backend and only
> sending protocol-level messages while maintaining a replication
> connection with a database, haven't you? This would be enough to
> warrant a backpatch to keep the state of the live connection clean.

Yes, I had not made any changes in backend.

> > The patch clears the pointers after streaming and error cleanup. For
> > physical streaming, I've also freed the reader before returning. Is there
> > a reason to leave that part to cmd_context reset instead?
>
> StartReplication() is complicated enough that I'd be OK with just
> adding an extra XLogReaderFree() there. It makes plainly clear that
> once we are done with the replication bytes, the xlogreader state
> allocated at the beginning of StartReplication() stored statically is
> not something we should rely on anymore.

Sounds good.

> > I've adjusted the assertion too, since it runs after the streaming
> > function returns. At that point the pointers are now NULL.
>
> I don't feel strongly about these additions though. What do they
> actually buy?

So just by looking at while I was clearing xlogreader, and the assert
in master branch said Assert(xlogreader != NULL);

The new assertion was meant to catch either streaming path leaving the
global pointers set after returning. It isn't needed for the fix, so
I'm fine with dropping it too..

Regards,
Ayush