Re: BUG #19684: Assertion in tuplesort_begin_heap() falsified by parallel plan with sort

Hi Ayush,

I reviewed v3 [1] on assert-enabled builds (meson, -Dcassert=true).

The fix looks correct to me. Apart from the optional test tweak in
4a, I think it is ready for a committer.

1. Master

I tested on master at 3c5d9d914fa with the reported query and ten
variants of it, all with the report's settings (cpu_tuple_cost = 1000,
min_parallel_table_scan_size = 1). Without the patch, six of them hit
Assert("nkeys > 0") in tuplesort_begin_heap(): the reported query, the
same query on populated tables with enable_hashagg = off, a three-way
UNION, UNION over a UNION ALL child, count(*) over the UNION, and the
same query with enable_parallel_append = off. With v3, none of them
crash and all return the expected rows. INTERSECT, EXCEPT and
UNION ... LIMIT 1 return the same results before and after, and a
one-column UNION still gets its Sort (Sort Key: t.i).

One plan change on master: for the populated-table query with
enable_hashagg left on, the plan goes from HashAggregate to Unique
over Gather, since there is no longer a Sort to pay for.

The new test does catch the bug: with only the test hunks of v3
applied to unpatched master, the union test crashes the server on the
same assertion.

v3 passes the main regression suite and test_plan_advice on master.

2. Back branches

The bug reproduces on REL_17_STABLE, REL_18_STABLE and REL_19_STABLE,
and it is easier to hit there than on master. With populated tables
and enable_hashagg left on, master chooses HashAggregate, but 17, 18
and 19 choose Unique -> Sort -> Gather and crash, which makes seven
crashing cases on each branch. With v3 applied, none of them crash and
the output matches master. The only plan change on the branches is
that the key-less Sort goes away (Unique -> Sort -> Gather becomes
Unique -> Gather).

The prepunion.c hunk applies to all three branches with only an
offset. The union.sql and union.out hunks do not apply as-is, because
the surrounding context differs. I put the same lines after the
"select from cte union select from cte" block, and the regression
suite passes on all three branches, as does test_plan_advice on 19.
17 and 18 have no test_plan_advice, so the comment about it and the
client_min_messages line could be dropped there.

Backpatch-through: 17 looks right: 66c0185a3 and 12933dc60 are in
REL_17_STABLE but not in REL_16_STABLE.

3. Assert(pathkeys != NIL) in create_sort_path()

I repeated Samriddha's experiment on v3, with the guard removed and
the Assert added. One thing to add to his result: the Assert fires
when the bad Sort path is built, not when it is chosen. So it trips
on plain EXPLAIN, and also on queries where that path loses on cost
and the executor never sees it (on master, the populated-table query
with enable_hashagg on, and UNION ... LIMIT 1). That makes it a
stricter check than the tuplesort assertion. Whether to add it seems
like a call for the committer.

4. The test

a) The expected output depends on max_parallel_workers_per_gather.
With max_parallel_workers_per_gather = 1 in a temp config, the new
test prints "Workers Planned: 1" and fails. Adding
"set max_parallel_workers_per_gather = 2" with a matching reset fixes
it. partition_prune already fails the same way under that setting, so
this is minor, but the test already sets the other planner settings
it relies on, and pinning this one too would make it self-contained.

b) The client_min_messages = error line is needed. Under
test_plan_advice, both the EXPLAIN and the SELECT otherwise emit
"WARNING: supplied plan advice was not enforced". A one-column UNION
gets the same warning on unpatched master, so the warning is not
caused by the fix, and the comment in the test is accurate.

Regards,
Trakshan

[1]
https://www.google.com/url?q=https://www.postgresql.org/message-id/CAJTYsWXnTQgH3m5a_bc4C%252BpumKj_fiEs4ZqaCjs8L8wvcrsy3g%2540mail.gmail.com&source=gmail&ust=1790696477151000&sa=E