On Fri, Oct 09, 2020 at 05:50:09PM -0300, Ranier Vilela wrote:
Em sex., 9 de out. de 2020 às 17:47, Tom Lane <t...@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier...@gmail.com> writes:
> The trap is not on the second part of expression. Is in the first.
> If the pointer is NULL, ExecCopySlot will be called.
Your initial comment indicated that you were worried about
IncrementalSortState's group_pivot slot, but that is never going
to be null in any execution function of nodeIncrementalSort.c,
because ExecInitIncrementalSort always creates it.
(The test whether it's NULL in ExecReScanIncrementalSort therefore
seems rather useless and misleading, but it's not actually a bug.)
The places that use TupIsNull are just doing so because that's
the standard way to check whether a slot is empty. The null
test inside the macro is pointless in this context (and in a lot
of its other use-cases, too) but we don't worry about that.
So I said that TupIsNull was not the most appropriate.
Doesn't it look better?
(node->group_pivot != NULL && TTS_EMPTY(node->group_pivot))
My (admittedly very subjective) opinion is that it looks much worse. The
TupIsNull is pretty self-descriptive, unlike this proposed code.
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.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
