On Fri, Oct 09, 2020 at 12:24:16PM -0300, Ranier Vilela wrote:
>I think that TupIsNull macro is no longer appropriate, to protect
>ExecCopySlot.
>
>See at tuptable.h:
>#define TupIsNull(slot) \
>((slot) == NULL || TTS_EMPTY(slot))
>
>If var node->group_pivot is NULL, ExecCopySlot will
>dereference a null pointer (first arg).
>
No. The C standard says there's a "sequence point" [1] between the left
and right arguments of the || operator, and that the expressions are
evaluated from left to right. So the program will do the first check,
and if the pointer really is NULL it won't do the second one (because
that is not necessary for determining the result). Similarly for the &&
operator, of course.
Really.
The trap is not on the second part of expression. Is in the first.
If the pointer is NULL, ExecCopySlot will be called.
For convenience, I will reproduce it:
static inline TupleTableSlot *
ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
{
Assert(!TTS_EMPTY(srcslot));
AssertArg(srcslot != dstslot);
dstslot->tts_ops->copyslot(dstslot, srcslot);
return dstslot;
}
The second arg is not empty? Yes.
The second arg is different from the first arg (NULL)? Yes.
dstslot->tts_ops->copyslot(dstslot, srcslot); // dereference dstslot (which is NULL)
Had this been wrong, surely some of the the other places TupIsNull would
be wrong too (and there are hundreds of them).
>Maybe, this can be related to a bug reported in the btree deduplication.
>
Not sure which bug you mean, but this piece of code is pretty unrelated
to btree in general, so I don't see any link.
Sorry, can't find the thread.
The author of deduplication claimed that he thinks the problem may be in IncrementalSort,
he did not specify which part.
regards,
Ranier Vilela