Skip to content

fix: a batch could be dropped without processing - #317

Open
imor wants to merge 1 commit into
rs/fix-cleanup-orderfrom
rs/fix-silent-batch-drop
Open

imor wants to merge 1 commit into
rs/fix-cleanup-orderfrom
rs/fix-silent-batch-drop

Conversation

@imor

@imor imor commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

If the first call to wait_event returned with an EINTR error, the continue statement jumped the control to the while (running_handles > 0) condition. Since running_handles was initialized to 0, it exited the loop and committed the open transaction, dropping the entire batch without processing it.

Since it is timing dependent and hard to write a deterministic test for, a test reproducing with sleeping is committed here: e73dd12

@imor
imor requested a review from a team as a code owner October 12, 2026 09:00
@imor
imor force-pushed the rs/fix-silent-batch-drop branch from 7e6eb95 to 4adbb35 Compare October 12, 2026 09:18
@imor
imor changed the base branch from rs/memory-barrier-fixes to rs/fix-cleanup-order October 12, 2026 09:19
@imor
imor added this pull request to stack #309 October 12, 2026 09:19
Comment thread src/worker.c Outdated
@steve-chavez

This comment was marked as outdated.

@steve-chavez steve-chavez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Disregard previous comment, just read the test.

@imor

imor commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

If the first call to wait_event returned with an EINTR error

Can you elaborate how do you trigger EINTR? I was only able to trigger that with gdb:

pg_net/src/worker.c

Lines 354 to 355 in b92fa5a

if (save_errno == EINTR) { // can happen when the wait is interrupted, for example when
// running under GDB. Just continue in this case.

If no real world condition triggers this, then it wouldn't be a fix strictly speaking.

There is a real world condition that triggers this, it's just very rare: when the worker is waiting in epoll_wait/kevent and a signal is delivered to the process it returns EINTR. Which means if someone was trying to reload the conf by sending a sighup, this condition would occur. That comment about GDB in the code is only partially correct, I'll update it. Even in the GDB case we'd want the code to not drop the batch.

@imor
imor force-pushed the rs/fix-silent-batch-drop branch from 4adbb35 to be28992 Compare October 13, 2026 03:36
@imor
imor force-pushed the rs/fix-silent-batch-drop branch 5 times, most recently from fdf32b3 to 8c3e4a2 Compare October 13, 2026 08:07
If the first call to wait_event returned with an EINTR error, the continue
statement jumped the control to the while (running_handles > 0) condition. Since
running_handles was initialized to 0, it exited the loop and committed the open
transaction, dropping the entire batch without processing it.
@imor
imor force-pushed the rs/fix-silent-batch-drop branch from 8c3e4a2 to 8d26ce4 Compare October 13, 2026 08:20
@AndrewJackson2020

Copy link
Copy Markdown
Contributor

Can we fix the spelling in the PR title? withour -> without

@imor imor changed the title fix: a batch could be dropped withour processing fix: a batch could be dropped without processing Oct 16, 2026
@imor

imor commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Can we fix the spelling in the PR title? withour -> without

Done

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants