Conversation
…t#1203) `fx` is module-scoped, so by the time test_a_query_that_cannot_use_the_order_does_not_pay_to_decide runs, its backend has absorbed the DDL of twenty-nine other tests. Both readings came out of that one connection, so how much catalog-cache state survived into each was not a property of the query being measured, and the difference between them had no upper bound and no fixed sign. That is the shape of the five failures in commandprompt#1203: two different arms, opposite directions, and one case of the same commit red then green. Each reading now opens its own connection. A backend that has just started carries a known amount: the first EXPLAIN fills its caches, the second measures the query, and what remains is the couple of buffers two fresh backends differ by -- which is what the tolerance of 5 was always sized for. NOT REPRODUCED LOCALLY, and the comment says so rather than implying a fix. Fourteen runs on a quiet box all gave a difference of exactly 0: full cluster corpus x5 on=283 off=283 diff=0 margin=21 this module x6 on=216 off=216 diff=0 margin=19 isolated, +600 tables, and a late-created table diff=0 in every rep invalidation injected between the two readings diff=0 in every rep So this bounds what a reading can carry; it does not turn an observed red green. THE ARM STILL DISCRIMINATES, proved by removal rather than argued. With the `has_useful_pathkeys` early return deleted from pgcolumnar_sorted_pathkeys: -- planning buffers, no ORDER BY: on=235 off=216 |on-off|=19 (tolerance 5) AssertionError: a query with no ORDER BY does not read the group list to decide: got 'differs by 19 (on=235 off=216 over 1000 groups)' want 'within 5' 1 failed, 47 passed The control passed in that run, which is the property that matters: the refusal cannot be what reddens when the guard goes. Check census unchanged at 1638. Full corpus 476 passed, three times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
|
Seventh occurrence, on #1198's head
That is worth having because it is the arm you could not reproduce in fourteen
One thing your run cannot have seen, since it is about the fixture rather than |
|
A sixth occurrence, on #1198 at
And it shows the contamination is one-sided and upward, which is the whole A catalog-cache miss or an invalidation can only ADD planning buffers. Nothing That is exactly the unbounded, unsigned quantity this PR removes by giving each It is also the second time this has reddened a PR that could not have caused it. |
jdatcmd
left a comment
There was a problem hiding this comment.
Approved on 1ecf40bac, and I checked the load-bearing claim at the source rather than taking the sign argument on trust.
pgcolumnar_sorted_pathkeys() opens with if (!pgcolumnar_enable_sorted_pathkeys) return NIL; as its first statement, before Relation r is ever opened. So the off arm does strictly less work than on, and a reading where off costs 69 buffers MORE has no physical interpretation. That settles it as an instrument defect rather than a mis-sized tolerance, which is what decides whether this change is the right shape.
One thing I checked because the sibling does it and this does not: test_catalog_plan_index.py sets search_path explicitly on its fresh reader, and this does not. It is fine here, and for a reason rather than by luck — fx connects on the bare pgc_cluster.dsn() rather than through pgc_conn, so pb lives in public and a new backend on the same DSN resolves it. Worth knowing that the two fixtures differ, because the same edit under pgc_conn would land in a private per-test schema and fail.
Independently run here, PG18 assert build: full module 48 passed / 127 checks, and the arm itself five consecutive times green. That is not a reproduction of the flake and I am not offering it as one — your heading is right to say NOT REPRODUCED. It is a check that the new regime does not introduce a failure of its own.
The comment is honest about what it did and did not establish, which is the part I would have asked for if it were missing.
What this changes
_planning_buffersnow opens its own connection for each reading.fxis module-scoped, so by the timetest_a_query_that_cannot_use_the_order_does_not_pay_to_decideruns, its backendhas absorbed the DDL of twenty-nine other tests in the file. Both readings came
out of that one connection, so how much catalog-cache state survived into each
was not a property of the query being measured — and the difference between two
such readings has no upper bound and no fixed sign.
That is the shape of the five failures in #1203: two different arms, opposite
directions, and one case of the same commit red then green.
A backend that has just started carries a known amount instead. The first EXPLAIN
fills its caches, the second measures the query, and what remains is the couple of
buffers two fresh backends differ by — which is what the tolerance of 5 was always
sized for.
What I could NOT do, stated plainly
I did not reproduce the flake. Fourteen runs on a quiet box, all giving a
difference of exactly 0:
on=283 off=283diff 0, margin 21on=216 off=216diff 0, margin 19on=218 off=218diff 0on=218 off=218diff 0on=219 off=219diff 0So this is a change of regime argued from the mechanism, not a red turned green.
Three hypotheses were tested and refuted: intrinsic noise, catalog size and
row position, and an injected invalidation. What remains is the one structural
difference I can name between a reading taken from a long-lived backend and one
taken from a fresh one, and only the second has a bound.
The arm still discriminates, proved by removal
With the
has_useful_pathkeysearly return deleted frompgcolumnar_sorted_pathkeys, rebuilt and re-run:The control passed in that run, which is the property that matters: the arm
that reddens is the one under test, not the control.
Two things worth recording that are not this change
cannot_runwould not have helped, and #1203 proposed it — I was wrong. Theissue suggested a failed control should refuse the run rather than fail the
suite. In this harness
cannot_runends the runEXIT_INCOMPLETEby design, so arefusal is also a red job. It renames an unattributable red rather than removing
one. I have said so on the issue.
The documented effect size may be optimistic. The tolerance comment says the
unguarded arm read
+44on this fixture. My removal proof measured+19. Stillalmost four times the tolerance, so the arm works — but a reader sizing a future
change against
+44has more headroom in mind than I could measure. Not changedhere, because I have one measurement and the comment may be from a configuration
I did not reproduce.
Scope
The two harnesses stay independent: this touches the pytest file only. The shell
twin's equivalent arm already starts a new backend per reading, which is why its
regime is the deterministic one; nothing here calls it or is called by it.
Check census unchanged at 1638. Full corpus 476 passed, three times with the fix.
Closes #1203
🤖 Generated with Claude Code
https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP