Re: a thinko in b676ac443b6

On 7/27/21 4:28 AM, Amit Langote wrote:
> Hi,
> 
> I noticed $subject while rebasing my patch at [1] to enable batching
> for the inserts used in cross-partition UPDATEs.
> 
> b676ac443b6 did this:
> 
> -           resultRelInfo->ri_PlanSlots[resultRelInfo->ri_NumSlots] =
> -               MakeSingleTupleTableSlot(planSlot->tts_tupleDescriptor,
> -                                        planSlot->tts_ops);
> ...
> +           {
> +               TupleDesc tdesc =
> CreateTupleDescCopy(slot->tts_tupleDescriptor);
> +
> +               resultRelInfo->ri_Slots[resultRelInfo->ri_NumSlots] =
> +                   MakeSingleTupleTableSlot(tdesc, slot->tts_ops);
> ...
> +               resultRelInfo->ri_PlanSlots[resultRelInfo->ri_NumSlots] =
> +                   MakeSingleTupleTableSlot(tdesc, planSlot->tts_ops);
> 
> I think it can be incorrect to use the same TupleDesc for both the
> slots in ri_Slots (for ready-to-be-inserted tuples) and ri_PlanSlots
> (for subplan output tuples).  Especially if you consider what we did
> in 86dc90056df that was committed into v14.  In that commit, we
> changed the way a subplan under ModifyTable produces its output for an
> UPDATE statement.  Previously, it would produce a tuple matching the
> target table's TupleDesc exactly (plus any junk columns), but now it
> produces only a partial tuple containing the values for the changed
> columns.
> 
> So it's better to revert to using planSlot->tts_tupleDescriptor for
> the slots in ri_PlanSlots.  Attached a patch to do so.
> 

Yeah, this seems like a clear mistake - thanks for noticing it! Clearly
no regression test triggered the issue, so I wonder what's the best way
to test it - any idea what would the test need to do?

I did some quick experiments with batched INSERTs with RETURNING clauses
and/or subplans, but I haven't succeeded in triggering the issue :-(


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company