[PATCH] Clear walsender reader pointers after cleanup
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t254142psql -h localhost -U postgresBuilt from patchset v1 (message #1), October 13, 2026 at 02:46 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t254142_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t254142_1 && git checkout t254142_1Patchset v1 (message #1) is on t254142_1
Hi,
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.
I tried sending CopyDone, draining the replies, then TIMELINE_HISTORY 99.
With --enable-cassert, I get close(0x7F7F7F7F). The connection stays up,
but the attached patch stops that invalid close.
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?
I've adjusted the assertion too, since it runs after the streaming
function returns. At that point the pointers are now NULL.
Regards,
Ayush
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.
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.
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?
--
Michael
Hi,
On Tue, 13 Oct 2026 at 04:35, Michael Paquier <michael@paquier.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
On Mon, Oct 12, 2026 at 06:30:52PM -0700, Ayush Tiwari wrote:
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..
Hmm. I was looking at all that again, and at the end I have moved
the assertion added by 879ad9f90e83 to be checked at the beginning of
the execution of a replication command, extending it to also check
after the logical replication context.
And done as of f2dd2a3bd44e.
--
Michael