Re: Gather Merge

On Fri, Mar 10, 2017 at 7:59 AM, Rushabh Lathia
<rushabh(dot)lathia(at)gmail(dot)com> wrote:
> Error coming from create_gather_merge_plan() from below condition:
>
> if (memcmp(sortColIdx, gm_plan->sortColIdx,
> numsortkeys * sizeof(AttrNumber)) != 0)
> elog(ERROR, "GatherMerge child's targetlist doesn't match
> GatherMerge");
>
> Above condition checks the sort column numbers explicitly, to ensure the
> tlists
> really do match up. This been copied from the create_merge_append_plan().
> Now
> this make sense as for MergeAppend as its not projection capable plan (see
> is_projection_capable_plan()). But like Gather, GatherMerge is the
> projection
> capable and its target list can be different from the subplan, so I don't
> think this
> condition make sense for the GatherMerge.
>
> Here is the some one the debugging info, through which I was able to reach
> to this conclusion:
>
> - targetlist for the GatherMerge and subpath is same during
> create_gather_merge_path().
>
> - targetlist for the GatherMerge is getting changed into
> create_gather_merge_plan().
>
> postgres=# explain (analyze, verbose) select t2.j from t1 JOIN t2 ON
> t1.k=t2.k where t1.i=1 order by t1.j desc;
> NOTICE: path parthtarget: {PATHTARGET :exprs ({VAR :varno 2 :varattno 2
> :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 2 :varoattno
> 2 :location 34} {VAR :varno 1 :varattno 2 :vartype 23 :vartypmod -1
> :varcollid 0 :varlevelsup 0 :varnoold 1 :varoattno 2 :location 90})
> :sortgrouprefs 0 1 :cost.startup 0.00 :cost.per_tuple 0.00 :width 8}
>
> NOTICE: subpath parthtarget: {PATHTARGET :exprs ({VAR :varno 1 :varattno 2
> :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 1 :varoattno
> 2 :location 90} {VAR :varno 2 :varattno 2 :vartype 23 :vartypmod -1
> :varcollid 0 :varlevelsup 0 :varnoold 2 :varoattno 2 :location 34})
> :cost.startup 0.00 :cost.per_tuple 0.00 :width 8}
>
> - Attached memory watch point and found that target list for GatherMerge is
> getting
> changed into groupping_planner() -> apply_projection_to_path().
>
> PFA patch to fix this issue.

I don't think this fix is correct, partly on theoretical grounds and
partly because I managed to make it crash. The problem is that
prepare_sort_for_pathkeys() actually alters the output tlist of Gather
Merge, which is inconsistent with the idea that Gather Merge can do
projection. It's going to produce whatever
prepare_sort_for_pathkeys() says it's going to produce, which may or
may not be what was there before. Using Kuntal's dump file and your
patch:

set min_parallel_table_scan_size = 0;
set parallel_setup_cost = 0;
set parallel_tuple_cost = 0;
set enable_sort = false;
set enable_bitmapscan = false;
alter table t1 alter column j type text;
select t2.i from t1 join t2 on t1.k=t2.k where t1.i=1 order by t1.j desc;

Crash. Abbreviated stack trace:

#0 pg_detoast_datum_packed (datum=0xbc) at fmgr.c:2176
#1 0x000000010160e707 in varstrfastcmp_locale (x=188, y=819,
ssup=0x7fe1ea06a568) at varlena.c:1997
#2 0x00000001013efc73 in ApplySortComparator [inlined] () at
/Users/rhaas/pgsql/src/include/utils/sortsupport.h:225
#3 0x00000001013efc73 in heap_compare_slots (a=<value temporarily
unavailable, due to optimizations>, b=<value temporarily unavailable,
due to optimizations>, arg=0x7fe1ea04e590) at sortsupport.h:681
#4 0x00000001014057b2 in sift_down (heap=0x7fe1ea079458,
node_off=<value temporarily unavailable, due to optimizations>) at
binaryheap.c:274
#5 0x000000010140573a in binaryheap_build (heap=0x7fe1ea079458) at
binaryheap.c:131
#6 0x00000001013ef771 in gather_merge_getnext [inlined] () at
/Users/rhaas/pgsql/src/backend/executor/nodeGatherMerge.c:421
#7 0x00000001013ef771 in ExecGatherMerge (node=0x7fe1ea04e590) at
nodeGatherMerge.c:240

Obviously, this is happening because we're trying to apply a
comparator for text to a value of type integer. I propose the
attached, slightly more involved fix, which rips out the first call to
prepare_sort_from_pathkeys() altogether.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company