Re: [HACKERS] [POC] Faster processing at Gather node

On Fri, Nov 10, 2017 at 8:39 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Nov 10, 2017 at 5:44 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> I am seeing the assertion failure as below on executing the above
>> mentioned Create statement:
>>
>> TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
>> "heapam.c", Line: 2634)
>> server closed the connection unexpectedly
>> This probably means the server terminated abnormally
>
> OK, I see it now. Not sure why I couldn't reproduce this before.
>
> I think the problem is not actually with the code that I just wrote.
> What I'm seeing is that the slot descriptor's tdhasoid value is false
> for both the funnel slot and the result slot; therefore, we conclude
> that no projection is needed to remove the OIDs. That seems to make
> sense: if the funnel slot doesn't have OIDs and the result slot
> doesn't have OIDs either, then we don't need to remove them.
> Unfortunately, even though the funnel slot descriptor is marked
> tdhashoid = false, the tuples being stored there actually do have
> OIDs. And that is because they are coming from the underlying
> sequential scan, which *also* has OIDs despite the fact that tdhasoid
> for it's slot is false.
>
> This had me really confused until I realized that there are two
> processes involved. The problem is that we don't pass eflags down to
> the child process -- so in the user backend, everybody agrees that
> there shouldn't be OIDs anywhere, because EXEC_FLAG_WITHOUT_OIDS is
> set. In the parallel worker, however, it's not set, so the worker
> feels free to do whatever comes naturally, and in this test case that
> happens to be returning tuples with OIDs. Patch for this attached.
>
> I also noticed that the code that initializes the funnel slot is using
> its own PlanState rather than the outer plan's PlanState to call
> ExecContextForcesOids. I think that's formally incorrect, because the
> goal is to end up with a slot that is the same as the outer plan's
> slot. It doesn't matter because ExecContextForcesOids doesn't care
> which PlanState it gets passed, but the comments in
> ExecContextForcesOids imply that somebody it might, so perhaps it's
> best to clean that up. Patch for this attached, too.
>

- if (!ExecContextForcesOids(&gatherstate->ps, &hasoid))
+ if (!ExecContextForcesOids(outerPlanState(gatherstate), &hasoid))
hasoid = false;

Don't we need a similar change in nodeGatherMerge.c (in function
ExecInitGatherMerge)?

> And here are the other patches again, too.
>

The 0001* patch doesn't apply, please find the attached rebased
version which I have used to verify the patch.

Now, along with 0001* and 0002*, 0003-skip-gather-project-v2 looks
good to me. I think we can proceed with the commit of 0001*~0003*
patches unless somebody else has any comments.

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