Repository navigation
Fix flaky diskquota primary failure test - #2117
krylosov-aa wants to merge 1 commit into
Conversation
|
Can you rebase it please? |
| IF reached THEN | ||
| RETURN true; | ||
| END IF; | ||
| IF clock_timestamp() >= deadline THEN |
There was a problem hiding this comment.
Could we include the expected state and the actual content 0 segment configuration in the timeout error instead of just returning false?
There was a problem hiding this comment.
Done. On timeout, wait_for_content0_state() now raises an error with the expected state and the current content 0 rows from gp_segment_configuration
| plpy.error('Invalid recovery command') | ||
| process = subprocess.Popen(['gprecoverseg', '-' + command, '-d', datadir], | ||
| stdout=subprocess.PIPE, stderr=subprocess.STDOUT) | ||
| output = process.communicate()[0] |
There was a problem hiding this comment.
What happens if statement_timeout fires while we're waiting in communicate()?
Could we verify that cancelling this statement also terminates and reaps the gprecoverseg process before the test continues? There is no subprocess cleanup around communicate().
There was a problem hiding this comment.
In the previous revision the recovery was aborted midway rather than left running. StatementTimeoutHandler sends SIGINT to the backend's whole process group, and gprecoverseg and its local helpers were in that group.
Cleaning up from Python after the cancel isn't reliable either. PL/Python turns a cancel into a KeyboardInterrupt through Py_AddPendingCall(). The statement timeout signals the backend twice, so a second KeyboardInterrupt is raised as soon as the handler for the first one is entered.
The new revision bounds the command instead: pg_recoverseg() runs timeout -k 60 <timeout_seconds> gprecoverseg. timeout(1) puts gprecoverseg and its local children in a separate process group, so the statement timeout's SIGINT doesn't reach them. When the limit expires, it sends SIGTERM, which gprecoverseg handles without a prompt, and SIGKILL 60 seconds later.
The trade-off is that a cancel no longer interrupts the recovery: the backend handles it only after gprecoverseg exits, either on its own or through timeout(1). Either way, the test can't move on while gprecoverseg is still running. If the limit is hit, recovery stops midway and gprecoverseg requires -F next time; the test then fails with the gprecoverseg output.
4dd7b78 to
5337721
Compare
|
Thanks for the explanation! I checked the current PR head (5337721), but still see the direct gprecoverseg call and RETURN false on timeout. Could the changes you described be missing from the push? |
The test could run queries before failover was ready or start rebalance before recovery had finished. Wait for the expected segment state and a successful distributed query, and wait for synchronization before rebalance.
5337721 to
c61b9a6
Compare
|
@Alena0704 Ouch, my bad) Pushed it now |
|
While testing this PR, I found that statement_timeout can sometimes leave a zombie timeout process. The root cause is still unclear, but I don’t think this should block merging the PR. I’ve drafted a separate issue with a minimal reproducer, prepared with AI assistance. |
|
I’ve encountered this bug too. But I haven’t figured out the cause yet either. |
What does this PR do?
The test could run queries before failover was ready or start rebalance before recovery had finished. Wait for the expected segment state and a successful distributed query, and wait for synchronization before rebalance.
Type of Change
Test Plan
Local checks on Linux ARM64
Checklist