Hi, I have started reviewing the patch and doing some testing, and I have pretty quickly ran into a segfault. Attached is a simple reproducer and an backtrace. AFAICS the bug seems to be somewhere in the tuplesort changes, likely resetting a memory context too soon or something like that. I haven't investigated it further, but it matches my hunch that tuplesort is likely where the bugs will be.
Otherwise the patch seems fairly complete. A couple of minor things that
I noticed while eyeballing the changes in a diff editor.
1) On a couple of places the new code has this comment
/* even when not parallel-aware */
while all the immediately preceding blocks use
/* even when not parallel-aware, for EXPLAIN ANALYZE */
I suggest using the same comment, otherwise it kinda suggests it's not
because of EXPLAIN ANALYZE.
2) I think the purpose of sampleSlot should be explicitly documented
(and I'm not sure "sample" is a good term here, as is suggest some sort
of sampling (for example nodeAgg uses grp_firstTuple).
3) skipCols/SkipKeyData seems a bit strange too, I think. I'd use
PresortedKeyData or something like that.
4) In cmpSortSkipCols, when checking if the columns changed, the patch
does this:
n = ((IncrementalSort *) node->ss.ps.plan)->skipCols;
for (i = 0; i < n; i++)
{
... check i-th key ...
}
My hunch is that checking the keys from the last one, i.e.
for (i = (n-1); i >= 0; i--)
{
....
}
would be faster. The reasoning is that with "ORDER BY a,b" the column
"b" changes more often. But I've been unable to test this because of the
segfault crashes.
5) The changes from
if (pathkeys_contained_in(...))
to
n = pathkeys_common(pathkeys, subpath->pathkeys);
if (n == 0)
seem rather inconvenient to me, as it makes the code unnecessarily
verbose. I wonder if there's a better way to deal with this.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
test2=# EXPLAIN SELECT * FROM (SELECT * FROM t ORDER BY a) foo ORDER BY a, b;
QUERY PLAN
------------------------------------------------------------------------
Incremental Sort (cost=147027.04..269419.25 rows=1000000 width=20)
Sort Key: t.a, t.b
Presorted Key: t.a
-> Sort (cost=136537.84..139037.84 rows=1000000 width=20)
Sort Key: t.a
-> Seq Scan on t (cost=0.00..16370.00 rows=1000000 width=20)
(6 rows)
crash.sql
Description: application/sql
Core was generated by `postgres: tomas test2 [local] SELECT
'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 MemoryContextResetOnly (context=0x0) at mcxt.c:158
158 if (!context->isReset)
(gdb) bt
#0 MemoryContextResetOnly (context=0x0) at mcxt.c:158
#1 0x0000560f622f07b3 in tuplesort_reset (state=0x560f632e56a0) at
tuplesort.c:1391
#2 0x0000560f620a2d17 in ExecIncrementalSort (pstate=0x560f632e1890) at
nodeIncrementalSort.c:280
#3 0x0000560f6207d5da in ExecProcNode (node=0x560f632e1890) at
../../../src/include/executor/executor.h:239
#4 ExecutePlan (execute_once=<optimized out>, dest=0x560f632daf28,
direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>,
operation=CMD_SELECT, use_parallel_mode=<optimized out>,
planstate=0x560f632e1890, estate=0x560f632e1680) at execMain.c:1721
#5 standard_ExecutorRun (queryDesc=0x560f63233f60, direction=<optimized out>,
count=0, execute_once=<optimized out>) at execMain.c:361
#6 0x0000560f621b8cfc in PortalRunSelect (portal=portal@entry=0x560f63277e40,
forward=forward@entry=1 '\001', count=0, count@entry=9223372036854775807,
dest=dest@entry=0x560f632daf28) at pquery.c:932
#7 0x0000560f621ba136 in PortalRun (portal=portal@entry=0x560f63277e40,
count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=1 '\001',
run_once=run_once@entry=1 '\001', dest=dest@entry=0x560f632daf28,
altdest=altdest@entry=0x560f632daf28,
completionTag=0x7fffdd875ac0 "") at pquery.c:773
#8 0x0000560f621b60cb in exec_simple_query (query_string=0x560f63213820
"SELECT * FROM (SELECT * FROM t ORDER BY a) foo ORDER BY a, b;") at
postgres.c:1120
#9 0x0000560f621b7df4 in PostgresMain (argc=<optimized out>,
argv=argv@entry=0x560f63240678, dbname=<optimized out>, username=<optimized
out>) at postgres.c:4144
#10 0x0000560f61ef56d1 in BackendRun (port=0x560f63236130) at postmaster.c:4412
#11 BackendStartup (port=0x560f63236130) at postmaster.c:4084
#12 ServerLoop () at postmaster.c:1757
#13 0x0000560f62148242 in PostmasterMain (argc=3, argv=0x560f6320e3e0) at
postmaster.c:1365
#14 0x0000560f61ef66b7 in main (argc=3, argv=0x560f6320e3e0) at main.c:228
