Thread: a thinko in b676ac443b6
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/
Attachment
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
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
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