Re: [HACKERS] Gather Merge

On Fri, Feb 17, 2017 at 6:27 PM, Rushabh Lathia
<rushabh.lathia@gmail.com> wrote:
> On Fri, Feb 17, 2017 at 4:47 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>
>> Are you suggesting that commit 0c2070ce has helped to improve
>> performance if so, I don't think that has been proved?  I guess the
>> numbers are different either due to different m/c or some other
>> settings like scale factor or work_mem.
>
>
> I don't really think 0c2070ce is the exact reason. I run the tpch runs
> with the same same setting as what Robert was running. I haven't
> noticed any regression with the runs. For the last runs I also
> uploaded the tpch run outputs for the individual queries for the
> reference.
>

Okay, then I am not sure why you and Robert are seeing different
results, probably because you are using a different machine.  Another
thing which we might need to think about this patch is support of
mark/restore position.  As of now the paths which create data in
sorted order like sort node and indexscan have support for
mark/restore position.   This is required for correctness when such a
node appears on the inner side of Merge Join.  Even though this patch
doesn't support mark/restore, it will not produce wrong results
because planner inserts Materialize node to compensate for it, refer
below code.

final_cost_mergejoin()
{
..
/*
* Even if materializing doesn't look cheaper, we *must* do it if the
* inner path is to be used directly (without sorting) and it doesn't
* support mark/restore.
*
* Since the inner side must be ordered, and only Sorts and IndexScans can
* create order to begin with, and they both support mark/restore, you
* might think there's no problem --- but you'd be wrong.  Nestloop and
* merge joins can *preserve* the order of their inputs, so they can be
* selected as the input of a mergejoin, and they don't support
* mark/restore at present.
*
* We don't test the value of enable_material here, because
* materialization is required for correctness in this case, and turning
* it off does not entitle us to deliver an invalid plan.
*/
else if (innersortkeys == NIL &&
!ExecSupportsMarkRestore(inner_path))
path->materialize_inner = true;
..
}

I think there is a value in supporting mark/restore position for any
node which produces sorted results, however, if you don't want to
support it, then I think we should update above comment in the code to
note this fact.  Also, you might want to check other places to see if
any similar comment updates are required in case you don't want to
support mark/restore position for GatherMerge.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com