On Sat, 4 Nov 2023 at 15:15, Andres Freund <and...@anarazel.de> wrote:
>
> On 2023-11-01 11:35:50 +1300, David Rowley wrote:
> > I changed the Assert in tts_virtual_copyslot() to check the natts
> > match in each of the slots and all of the regression tests still pass,
> > so it seems we have no tests where there's an attribute number
> > mismatch...
>
> If we want to prohibit that, I think we ought to assert this in
> ExecCopySlot(), rather than just tts_virtual_copyslot.
>
> Even that does survive the test - but I don't think it'd be really wrong to
> copy from a slot with more columns into one with fewer. And it seems plausible
> that that could happen somewhere, e.g. when copying from a slot in a child
> partition with additional columns into a slot from the parent, where the
> column types/order otherwise matches, so we don't have to use the attribute
> mapping infrastructure.
Do you have any examples of when this could happen?
I played around with partitioned tables and partitions with various
combinations of dropped columns and can't see any cases of this. Given
the assert's condition has been backwards for 5 years now
(4da597edf1b), it just seems a bit unlikely that we have cases where
the source slot can have more attributes than the destination.
Given the Assert has been that way around for this long without any
complaints, I think we should just insist that the natts must match in
each slot. If we later discover some reason that there's some corner
case where they don't match, we can adjust the code then.
I played around with the attached patch which removes the Assert and
puts some additional Assert checks inside ExecCopySlot() which
additionally checks the attribute types also match. There are valid
cases where they don't match and that seems to be limited to cases
where we're performing DML on a table with a dropped column.
expand_insert_targetlist() will add NULL::int4 constants to the
targetlist in place of dropped columns but the tupledesc of the table
will have the atttypid set to InvalidOid per what that gets set to
when a column is dropped in RemoveAttributeById().
> > I think if we are going to support copying slots where the source and
> > destination don't have the same number of attributes then the
> > following comment should explain what's allowed and what's not
> > allowed:
> >
> > /*
> > * Copy the contents of the source slot into the destination slot's own
> > * context. Invoked using callback of the destination slot.
> > */
> > void (*copyslot) (TupleTableSlot *dstslot, TupleTableSlot *srcslot);
>
> Arguably the more relevant place would be document this in ExecCopySlot(), as
> that's what "users" of ExecCopySlot() would presumably would look at. I dug a
> bit in the history, and we used to say
I think it depends on what you're documenting. Writing comments above
the copyslot API function declaration is useful to define the API
standard for what new implementations of that interface must abide by.
Comments in ExecCopySlot() are useful to users of that function. It
seems to me that both locations are relevant. New implementations of
copyslot need to know what they must handle, else they're left just to
look at what other implementations do and guess the rest.
David
diff --git a/src/backend/executor/execTuples.c
b/src/backend/executor/execTuples.c
index 2c2712ceac..5a2db83987 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -253,8 +253,6 @@ tts_virtual_copyslot(TupleTableSlot *dstslot,
TupleTableSlot *srcslot)
{
TupleDesc srcdesc = srcslot->tts_tupleDescriptor;
- Assert(srcdesc->natts <= dstslot->tts_tupleDescriptor->natts);
-
tts_virtual_clear(dstslot);
slot_getallattrs(srcslot);
diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index 4210d6d838..cc32550840 100644
--- a/src/include/executor/tuptable.h
+++ b/src/include/executor/tuptable.h
@@ -174,7 +174,10 @@ struct TupleTableSlotOps
/*
* Copy the contents of the source slot into the destination slot's own
- * context. Invoked using callback of the destination slot.
+ * context. Invoked using callback of the destination slot. 'dstslot'
and
+ * 'srcslot' can be assumed to match in terms of attribute count and
data
+ * types. However, either TupleDesc may sometimes contain attribute
types
+ * of InvalidOid in cases where the attribute refers to a dropped
column.
*/
void (*copyslot) (TupleTableSlot *dstslot, TupleTableSlot
*srcslot);
@@ -340,6 +343,35 @@ extern void slot_getsomeattrs_int(TupleTableSlot *slot,
int attnum);
#ifndef FRONTEND
+
+/*
+ * VerifySlotCompatibility
+ * Ensure the two given slots are compatible with one another in
terms of
+ * their number of attributes and attribute types.
+ */
+static void
+VerifySlotCompatibility(TupleTableSlot *slot1, TupleTableSlot *slot2)
+{
+#ifdef USE_ASSERT_CHECKING
+ TupleDesc desc1 = slot1->tts_tupleDescriptor;
+ TupleDesc desc2 = slot2->tts_tupleDescriptor;
+
+ Assert(desc1->natts == desc2->natts);
+
+ for (int i = 0; i < desc1->natts; i++)
+ {
+ Oid type1 = desc1->attrs[i].atttypid;
+ Oid type2 = desc2->attrs[i].atttypid;
+
+ /*
+ * Ensure the attribute types match. InvalidOid is permitted
as we can end
+ * up with dropped columns in a TupleDesc
+ */
+ Assert(type1 == type2 || !OidIsValid(type1) ||
!OidIsValid(type2));
+ }
+#endif
+}
+
/*
* This function forces the entries of the slot's Datum/isnull arrays to be
* valid at least up through the attnum'th entry.
@@ -477,6 +509,9 @@ ExecCopySlotMinimalTuple(TupleTableSlot *slot)
*
* If a source's system attributes are supposed to be accessed in the target
* slot, the target slot and source slot types need to match.
+ *
+ * The source and destination slot's tuple descriptor must match in terms of
+ * attribute types and number of attributes.
*/
static inline TupleTableSlot *
ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
@@ -484,6 +519,8 @@ ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot
*srcslot)
Assert(!TTS_EMPTY(srcslot));
Assert(srcslot != dstslot);
+ VerifySlotCompatibility(dstslot, srcslot);
+
dstslot->tts_ops->copyslot(dstslot, srcslot);
return dstslot;
