Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> My (admittedly very subjective) opinion is that it looks much worse. The
> TupIsNull is pretty self-descriptive, unlike this proposed code.

+1

> That could be fixed by defining a new macro, perhaps something like
> SlotIsEmpty() or so. But as was already explained, Incremental Sort
> can't actually have a NULL slot here, so it makes no difference there.
> And in the other places we can't just mechanically replace the macros
> because it'd quite likely silently hide pre-existing bugs.

IME, there are basically two use-cases for TupIsNull in the executor:

1. Checking whether a lower-level plan node has returned an actual
tuple or an EOF indicator.  In current usage, both parts of the
TupIsNull test are needed here, because some node types like to
return NULL pointers while others do something like
"return ExecClearTuple(myslot)".

2. Manipulating a locally-managed slot.  In just about every case
of this sort, the slot is created during the node Init function,
so that the NULL test in TupIsNull is unnecessary and what we are
really interested in is the empty-or-not state of the slot.

Thus, Ranier's concern would be valid if a node ever did anything
with a returned-from-lower-level slot after failing the TupIsNull
check on it.  But there's really no reason to do so, and furthermore
doing so would be a logic bug in itself.  (Something like ExecCopySlot
into the slot, for example, is flat out wrong, because an upper level
node is *never* entitled to scribble on the output slot of a lower-level
node.)  So I seriously, seriously doubt that there are any live bugs
of this ilk.

In principle we could invent SlotIsEmpty() and apply it in use
cases of type 2, but I don't really think that'd be a productive
activity.  In return for saving a few cycles we'd have a nontrivial
risk of new bugs from using the wrong macro for the case at hand.

I do wonder whether we should try to simplify the inter-node
API by allowing only one of the two cases for EOF indicators.
Not convinced it's worth troubling over, though.