Skip to content

Commit

Permalink
Fix usage of WaitEventSetWait() with timeout (#10947)
Browse files Browse the repository at this point in the history
WaitEventSetWait() returns the number of "events" that happened, and
only that many events in the WaitEvent array are updated. When the
timeout is reached, it returns 0 and does not modify the WaitEvent array
at all. We were reading 'event.events' without checking the return
value, which would be uninitialized when the timeout was hit.

No test included, as this is harmless at the moment. But this caused the
test I'm including in PR #10882 to fail. That PR changes the logic to
loop back to retry the PQgetCopyData() call if WL_SOCKET_READABLE was
set. Currently, making an extra call to PQconsumeInput() is harmless,
but with that change in logic, it turns into a busy-wait.
  • Loading branch information
hlinnaka authored Feb 24, 2025
1 parent df362de commit 40acb0c
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions pgxn/neon/libpagestore.c
Original file line number Diff line number Diff line change
Expand Up @@ -716,20 +716,21 @@ call_PQgetCopyData(shardno_t shard_no, char **buffer)

if (ret == 0)
{
WaitEvent event;
WaitEvent occurred_event;
int noccurred;
long timeout;

timeout = Max(0, LOG_INTERVAL_MS - INSTR_TIME_GET_MILLISEC(since_last_log));

/* Sleep until there's something to do */
(void) WaitEventSetWait(shard->wes_read, timeout, &event, 1,
WAIT_EVENT_NEON_PS_READ);
noccurred = WaitEventSetWait(shard->wes_read, timeout, &occurred_event, 1,
WAIT_EVENT_NEON_PS_READ);
ResetLatch(MyLatch);

CHECK_FOR_INTERRUPTS();

/* Data available in socket? */
if (event.events & WL_SOCKET_READABLE)
if (noccurred > 0 && (occurred_event.events & WL_SOCKET_READABLE) != 0)
{
if (!PQconsumeInput(pageserver_conn))
{
Expand Down

1 comment on commit 40acb0c

@github-actions
Copy link

Choose a reason for hiding this comment

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

7840 tests run: 7439 passed, 13 failed, 388 skipped (full report)


Failures on Postgres 16

  • test_throughput[github-actions-selfhosted-50-pipelining_config7-30-100-128-batchable {'max_batch_size': 1, 'execution': 'tasks', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config8-30-100-128-batchable {'max_batch_size': 2, 'execution': 'concurrent-futures', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config9-30-100-128-batchable {'max_batch_size': 2, 'execution': 'tasks', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config10-30-100-128-batchable {'max_batch_size': 4, 'execution': 'concurrent-futures', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config11-30-100-128-batchable {'max_batch_size': 4, 'execution': 'tasks', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config12-30-100-128-batchable {'max_batch_size': 8, 'execution': 'concurrent-futures', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config13-30-100-128-batchable {'max_batch_size': 8, 'execution': 'tasks', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config14-30-100-128-batchable {'max_batch_size': 16, 'execution': 'concurrent-futures', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config15-30-100-128-batchable {'max_batch_size': 16, 'execution': 'tasks', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config16-30-100-128-batchable {'max_batch_size': 32, 'execution': 'concurrent-futures', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config17-30-100-128-batchable {'max_batch_size': 32, 'execution': 'tasks', 'mode': 'pipelined'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config5-30-100-128-batchable {'mode': 'serial'}]: release-x86-64-with-lfc
  • test_throughput[github-actions-selfhosted-50-pipelining_config6-30-100-128-batchable {'max_batch_size': 1, 'execution': 'concurrent-futures', 'mode': 'pipelined'}]: release-x86-64-with-lfc
# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config7-30-100-128-batchable {'max_batch_size': 1, 'execution': 'tasks', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config8-30-100-128-batchable {'max_batch_size': 2, 'execution': 'concurrent-futures', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config9-30-100-128-batchable {'max_batch_size': 2, 'execution': 'tasks', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config10-30-100-128-batchable {'max_batch_size': 4, 'execution': 'concurrent-futures', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config11-30-100-128-batchable {'max_batch_size': 4, 'execution': 'tasks', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config12-30-100-128-batchable {'max_batch_size': 8, 'execution': 'concurrent-futures', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config13-30-100-128-batchable {'max_batch_size': 8, 'execution': 'tasks', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config14-30-100-128-batchable {'max_batch_size': 16, 'execution': 'concurrent-futures', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config15-30-100-128-batchable {'max_batch_size': 16, 'execution': 'tasks', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config16-30-100-128-batchable {'max_batch_size': 32, 'execution': 'concurrent-futures', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config17-30-100-128-batchable {'max_batch_size': 32, 'execution': 'tasks', 'mode': 'pipelined'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config5-30-100-128-batchable {'mode': 'serial'}] or test_throughput[release-pg16-github-actions-selfhosted-50-pipelining_config6-30-100-128-batchable {'max_batch_size': 1, 'execution': 'concurrent-futures', 'mode': 'pipelined'}]"
Flaky tests (6)

Postgres 17

Code coverage* (full report)

  • functions: 32.8% (8631 of 26294 functions)
  • lines: 48.6% (72620 of 149326 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
40acb0c at 2025-02-24T21:11:37.802Z :recycle:

Please sign in to comment.