Horton

Thread

Commits

  1. Use appropriate tuple descriptor in FDW batching

  1. a thinko in b676ac443b6

    Amit Langote <amitlangote09@gmail.com> — 2021-07-27T02:28:00Z

    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.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

[1] https://commitfest.postgresql.org/33/2992/

  2. Re: a thinko in b676ac443b6

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-07-27T16:07:54Z

    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

  3. Re: a thinko in b676ac443b6

    Amit Langote <amitlangote09@gmail.com> — 2021-07-28T01:15:34Z

    On Wed, Jul 28, 2021 at 1:07 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> On 7/27/21 4:28 AM, Amit Langote wrote:
> > 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?

Ah, I should've mentioned that this is only a problem if the original
query is an UPDATE.  With v14, only INSERTs can use batching and the
subplan does output a tuple matching the target table's TupleDesc in
their case, so the code seems to work fine.

As I said, I noticed a problem when rebasing my patch to allow
cross-partition UPDATEs to use batching for the inserts that are
performed internally to implement such UPDATEs.  The exact problem I
noticed is that the following Assert tts_virtual_copyslot() (via
ExecCopySlot called with an ri_PlanSlots[] entry) failed:

    Assert(srcdesc->natts <= dstslot->tts_tupleDescriptor->natts);

srcdesc in this case is a slot in ri_PlanSlots[] initialized with the
target table's TupleDesc (the "thinko") and dstslot is the slot that
holds subplan's output tuple ('planSlot' passed to ExecInsert).  As I
described in my previous email, dstslot's TupleDesc can be narrower
than the target table's TupleDesc in the case of an UPDATE, so the
Assert can fail in theory.

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

Yeah, no way to trigger this except UPDATEs.  It still seems like a
good idea to fix this in v14.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

  4. Re: a thinko in b676ac443b6

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-07-28T10:19:14Z

    On 7/28/21 3:15 AM, Amit Langote wrote:
> On Wed, Jul 28, 2021 at 1:07 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>> On 7/27/21 4:28 AM, Amit Langote wrote:
>>> 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?
> 
> Ah, I should've mentioned that this is only a problem if the original
> query is an UPDATE.  With v14, only INSERTs can use batching and the
> subplan does output a tuple matching the target table's TupleDesc in
> their case, so the code seems to work fine.
> 
> As I said, I noticed a problem when rebasing my patch to allow
> cross-partition UPDATEs to use batching for the inserts that are
> performed internally to implement such UPDATEs.  The exact problem I
> noticed is that the following Assert tts_virtual_copyslot() (via
> ExecCopySlot called with an ri_PlanSlots[] entry) failed:
> 
>     Assert(srcdesc->natts <= dstslot->tts_tupleDescriptor->natts);
> 
> srcdesc in this case is a slot in ri_PlanSlots[] initialized with the
> target table's TupleDesc (the "thinko") and dstslot is the slot that
> holds subplan's output tuple ('planSlot' passed to ExecInsert).  As I
> described in my previous email, dstslot's TupleDesc can be narrower
> than the target table's TupleDesc in the case of an UPDATE, so the
> Assert can fail in theory.
> 
>> I did some quick experiments with batched INSERTs with RETURNING clauses
>> and/or subplans, but I haven't succeeded in triggering the issue :-(
> 
> Yeah, no way to trigger this except UPDATEs.  It still seems like a
> good idea to fix this in v14.
> 

OK, thanks for the explanation. So it's benign in v14, but I agree it's
better to fix it there too. I'll get this sorted/pushed.

regards

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