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

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.

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.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Em sex., 9 de out. de 2020 às 14:05, Tomas Vondra <
tomas.vondra@2ndquadrant.com> escreveu:

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

Ranier Vilela <ranier.vf@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.

regards, tom lane

Em sex., 9 de out. de 2020 às 17:47, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

Ranier Vilela <ranier.vf@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))

regards,
Ranier Vilela

On Fri, Oct 9, 2020 at 1:28 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

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.

No I didn't.

--
Peter Geoghegan

Em sex., 9 de out. de 2020 às 17:58, Peter Geoghegan <pg@bowt.ie> escreveu:

On Fri, Oct 9, 2020 at 1:28 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

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.

No I didn't.

/messages/by-id/CAH2-Wz=Ae84z0PXTBc+SSGi9EC8nGKn9D16OP-dgH47Jcrv0Ww@mail.gmail.com
" On Tue, Jul 28, 2020 at 8:47 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:

This is very likely to be related to incremental sort because it's a"

Ranier Vilela

Greetings,

* Ranier Vilela (ranier.vf@gmail.com) wrote:

Em sex., 9 de out. de 2020 às 14:05, Tomas Vondra <
tomas.vondra@2ndquadrant.com> escreveu:

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).

[...]

The trap is not on the second part of expression. Is in the first.
If the pointer is NULL, ExecCopySlot will be called.

Yeah, that's interesting, and this isn't the only place there's a risk
of that happening, in doing a bit of review of TupIsNull() callers:

src/backend/executor/nodeGroup.c:

if (TupIsNull(firsttupleslot))
{
outerslot = ExecProcNode(outerPlanState(node));
if (TupIsNull(outerslot))
{
/* empty input, so return nothing */
node->grp_done = true;
return NULL;
}
/* Copy tuple into firsttupleslot */
ExecCopySlot(firsttupleslot, outerslot);

Seems that 'firsttupleslot' could possibly be a NULL pointer at this
point?

src/backend/executor/nodeWindowAgg.c:

/* Fetch next row if we didn't already */
if (TupIsNull(agg_row_slot))
{
if (!window_gettupleslot(agg_winobj, winstate->aggregatedupto,
agg_row_slot))
break; /* must be end of partition */
}

If agg_row_slot ends up being an actual NULL pointer, this looks likely
to end up resulting in a crash too.

/*
* If this is the very first partition, we need to fetch the first input
* row to store in first_part_slot.
*/
if (TupIsNull(winstate->first_part_slot))
{
TupleTableSlot *outerslot = ExecProcNode(outerPlan);

if (!TupIsNull(outerslot))
ExecCopySlot(winstate->first_part_slot, outerslot);
else
{
/* outer plan is empty, so we have nothing to do */
winstate->partition_spooled = true;
winstate->more_partitions = false;
return;
}
}

This seems like another risky case, since we don't check that
winstate->first_part_slot is a non-NULL pointer.

if (winstate->frameheadpos == 0 &&
TupIsNull(winstate->framehead_slot))
{
/* fetch first row into framehead_slot, if we didn't already */
if (!tuplestore_gettupleslot(winstate->buffer, true, true,
winstate->framehead_slot))
elog(ERROR, "unexpected end of tuplestore");
}

There's a few of these 'framehead_slot' cases, and then some with
'frametail_slot', all a similar pattern to above.

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)

Right, just to try and clarify further, the issue here is with this code:

if (TupIsNull(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

With TupIsNull defined as:

((slot) == NULL || TTS_EMPTY(slot))

That means we get:

if ((node->group_pivot) == NULL || TTS_EMPTY(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

Which means that if we reach this point with node->group_pivot as NULL,
then we'll pass that to ExecCopySlot() and eventually end up
dereferencing it and crashing.

I haven't tried to run back farther up to see if it's possible that
there's other checks which prevent node->group_pivot (and the other
cases) from actually being a NULL pointer by the time we reach this
code, but I agree that it seems to be a bit concerning to have the code
written this way- TupIsNull() allows the pointer *itself* to be NULL and
callers of it need to realize that (if nothing else by at least
commenting that there's other checks in place to make sure that it can't
end up actually being a NULL pointer when we're passing it to some other
function that'll dereference it).

As a side-note, this kind of further analysis and looking for other,
similar, cases that might be problematic is really helpful and important
to do whenever you come across a case like this, and will also lend a
bit more validation that this is really an issue and something we need
to look at and not a one-off mistake (which, as much as we'd like to
think we never make mistakes, isn't typically the case...).

Thanks,

Stephen

Ranier Vilela <ranier.vf@gmail.com> writes:

So I said that TupIsNull was not the most appropriate.

[ shrug... ] You're entitled to your opinion, but I see essentially
no value in running around and trying to figure out which TupIsNull
calls actually can see a null pointer and which never will. It'd
likely introduce bugs, it would certainly not remove any, and there's
no reason to believe that any meaningful performance improvement
could be gained.

(It's possible that the compiler can remove some of the useless
tests, so I'm satisfied to leave such micro-optimization to it.)

regards, tom lane

Em sex., 9 de out. de 2020 às 18:02, Stephen Frost <sfrost@snowman.net>
escreveu:

Greetings,

* Ranier Vilela (ranier.vf@gmail.com) wrote:

Em sex., 9 de out. de 2020 às 14:05, Tomas Vondra <
tomas.vondra@2ndquadrant.com> escreveu:

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).

[...]

The trap is not on the second part of expression. Is in the first.
If the pointer is NULL, ExecCopySlot will be called.

Yeah, that's interesting, and this isn't the only place there's a risk
of that happening, in doing a bit of review of TupIsNull() callers:

src/backend/executor/nodeGroup.c:

if (TupIsNull(firsttupleslot))
{
outerslot = ExecProcNode(outerPlanState(node));
if (TupIsNull(outerslot))
{
/* empty input, so return nothing */
node->grp_done = true;
return NULL;
}
/* Copy tuple into firsttupleslot */
ExecCopySlot(firsttupleslot, outerslot);

Seems that 'firsttupleslot' could possibly be a NULL pointer at this
point?

src/backend/executor/nodeWindowAgg.c:

/* Fetch next row if we didn't already */
if (TupIsNull(agg_row_slot))
{
if (!window_gettupleslot(agg_winobj, winstate->aggregatedupto,
agg_row_slot))
break; /* must be end of partition */
}

If agg_row_slot ends up being an actual NULL pointer, this looks likely
to end up resulting in a crash too.

/*
* If this is the very first partition, we need to fetch the first
input
* row to store in first_part_slot.
*/
if (TupIsNull(winstate->first_part_slot))
{
TupleTableSlot *outerslot = ExecProcNode(outerPlan);

if (!TupIsNull(outerslot))
ExecCopySlot(winstate->first_part_slot, outerslot);
else
{
/* outer plan is empty, so we have nothing to do */
winstate->partition_spooled = true;
winstate->more_partitions = false;
return;
}
}

This seems like another risky case, since we don't check that
winstate->first_part_slot is a non-NULL pointer.

if (winstate->frameheadpos == 0 &&
TupIsNull(winstate->framehead_slot))
{
/* fetch first row into framehead_slot, if we didn't
already */
if (!tuplestore_gettupleslot(winstate->buffer, true, true,
winstate->framehead_slot))
elog(ERROR, "unexpected end of tuplestore");
}

There's a few of these 'framehead_slot' cases, and then some with
'frametail_slot', all a similar pattern to above.

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)

Right, just to try and clarify further, the issue here is with this code:

if (TupIsNull(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

With TupIsNull defined as:

((slot) == NULL || TTS_EMPTY(slot))

That means we get:

if ((node->group_pivot) == NULL || TTS_EMPTY(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

Which means that if we reach this point with node->group_pivot as NULL,
then we'll pass that to ExecCopySlot() and eventually end up
dereferencing it and crashing.

I haven't tried to run back farther up to see if it's possible that
there's other checks which prevent node->group_pivot (and the other
cases) from actually being a NULL pointer by the time we reach this
code, but I agree that it seems to be a bit concerning to have the code
written this way- TupIsNull() allows the pointer *itself* to be NULL and
callers of it need to realize that (if nothing else by at least
commenting that there's other checks in place to make sure that it can't
end up actually being a NULL pointer when we're passing it to some other
function that'll dereference it).

As a side-note, this kind of further analysis and looking for other,
similar, cases that might be problematic is really helpful and important
to do whenever you come across a case like this, and will also lend a
bit more validation that this is really an issue and something we need
to look at and not a one-off mistake (which, as much as we'd like to
think we never make mistakes, isn't typically the case...).

Several places.
TupIsNull it looks like a minefield...

regards,
Ranier Vilela

Greetings,

* Ranier Vilela (ranier.vf@gmail.com) wrote:

Em sex., 9 de out. de 2020 às 18:02, Stephen Frost <sfrost@snowman.net>
escreveu:

As a side-note, this kind of further analysis and looking for other,
similar, cases that might be problematic is really helpful and important
to do whenever you come across a case like this, and will also lend a
bit more validation that this is really an issue and something we need
to look at and not a one-off mistake (which, as much as we'd like to
think we never make mistakes, isn't typically the case...).

Several places.
TupIsNull it looks like a minefield...

Is it though? Tom already pointed out that the specific case you were
concerned about isn't an issue- I'd encourage you to go review the other
cases that I found and see if you can find any cases where it's actually
going to result in a crash.

If there is such a case, then perhaps we should consider changing
things, but if not, then perhaps there isn't any need to make a change.
I do wonder if maybe some of those cases don't acutally need the
TupIsNull() check at all as we could prove that it won't be by the time
we reach that point, but it depends- and requires more review.

Thanks,

Stephen

On Fri, Oct 9, 2020 at 2:04 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

The author of deduplication claimed that he thinks the problem may be in IncrementalSort,
he did not specify which part.

No I didn't.

/messages/by-id/CAH2-Wz=Ae84z0PXTBc+SSGi9EC8nGKn9D16OP-dgH47Jcrv0Ww@mail.gmail.com

That thread is obviously totally unrelated to what you're talking
about. I cannot imagine how you made the connection. The only
commonality is the term "incremental sort".

Moreover, the point that I make in the thread that you link to is that
the bug in question could not possibly be related to the incremental
sort commit. That was an initial quick guess that I made that turned
out to be wrong.

--
Peter Geoghegan

On Fri, Oct 09, 2020 at 05:25:02PM -0300, Ranier Vilela wrote:

Em sex., 9 de out. de 2020 �s 14:05, Tomas Vondra <
tomas.vondra@2ndquadrant.com> escreveu:

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.

Ah, OK. Now I see what you meant. Well, yeah - calling ExecCopySlot with
NULL would be bad, but as others pointed out most of the call sites
don't really have the issue for other reasons.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 <tgl@sss.pgh.pa.us> escreveu:

Ranier Vilela <ranier.vf@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

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.

regards, tom lane

Em sex., 9 de out. de 2020 às 19:45, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

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.

The problem is not only in nodeIncrementalSort.c, but in several others
too, where people are using TupIsNull with ExecCopySlot.
I would call this a design flaw.
If (TupIsNull)
ExecCopySlot

The callers, think they are using TupIsNotNullAndEmpty.
If (TupIsNotNullAndEmpty)
ExecCopySlot

regards,
Ranier Vilela

On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

The problem is not only in nodeIncrementalSort.c, but in several others
too, where people are using TupIsNull with ExecCopySlot.
I would call this a design flaw.
If (TupIsNull)
ExecCopySlot

The callers, think they are using TupIsNotNullAndEmpty.
If (TupIsNotNullAndEmpty)
ExecCopySlot

IMO both names are problematic, too data value centric, not semantic.
TupIsValid for the name and negating the existing tests would help to at
least clear that part up. Then, things operating on invalid tuples would
be expected to know about both representations. In the case of
ExecCopySlot there is nothing it can do with a null representation of an
invalid tuple so it would have to fail if presented one. An assertion
seems sufficient.

David J.

Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston <
david.g.johnston@gmail.com> escreveu:

On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

The problem is not only in nodeIncrementalSort.c, but in several others
too, where people are using TupIsNull with ExecCopySlot.
I would call this a design flaw.
If (TupIsNull)
ExecCopySlot

The callers, think they are using TupIsNotNullAndEmpty.
If (TupIsNotNullAndEmpty)
ExecCopySlot

IMO both names are problematic, too data value centric, not semantic.
TupIsValid for the name and negating the existing tests would help to at
least clear that part up. Then, things operating on invalid tuples would
be expected to know about both representations. In the case of
ExecCopySlot there is nothing it can do with a null representation of an
invalid tuple so it would have to fail if presented one. An assertion
seems sufficient.

IHMO, assertion it is not the solution.

Steven suggested looking for some NULL pointer font above the calls.
I say that it is not necessary, there is no NULL pointer.
Whoever guarantees this is the combination, which for me is an assertion.

If (TupIsNull)
ExecCopySlot

It works as a subject, but in release mode.
It is the equivalent of:

If (TupIsNull)
Abort

The only problem for me is that we are running this assertion on the
clients' machines.

regards,
Ranier Vilela

On Sun, Oct 11, 2020 at 3:31 AM Ranier Vilela <ranier.vf@gmail.com> wrote:

Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston <
david.g.johnston@gmail.com> escreveu:

On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

The problem is not only in nodeIncrementalSort.c, but in several others
too, where people are using TupIsNull with ExecCopySlot.
I would call this a design flaw.
If (TupIsNull)
ExecCopySlot

The callers, think they are using TupIsNotNullAndEmpty.
If (TupIsNotNullAndEmpty)
ExecCopySlot

IMO both names are problematic, too data value centric, not semantic.
TupIsValid for the name and negating the existing tests would help to at
least clear that part up. Then, things operating on invalid tuples would
be expected to know about both representations. In the case of
ExecCopySlot there is nothing it can do with a null representation of an
invalid tuple so it would have to fail if presented one. An assertion
seems sufficient.

IHMO, assertion it is not the solution.

Steven suggested looking for some NULL pointer font above the calls.
I say that it is not necessary, there is no NULL pointer.
Whoever guarantees this is the combination, which for me is an assertion.

If (TupIsNull)
ExecCopySlot

It works as a subject, but in release mode.
It is the equivalent of:

If (TupIsNull)
Abort

The only problem for me is that we are running this assertion on the
clients' machines.

I cannot make heads nor tails of what you are trying to communicate here.

I'll agree that TupIsNull isn't the most descriptive choice of name, and is
probably being abused throughout the code base, but the overall intent and
existing flow seems fine. My only goal would be to make it a bit easier
for unfamiliar coders to pick up on the coding pattern and assumptions and
make coding errors there more obvious. Renaming and/or an assertion fits
that goal. Breaking the current abstraction level doesn't seem desirable.

David J.

Em dom., 11 de out. de 2020 às 14:53, David G. Johnston <
david.g.johnston@gmail.com> escreveu:

On Sun, Oct 11, 2020 at 3:31 AM Ranier Vilela <ranier.vf@gmail.com> wrote:

Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston <
david.g.johnston@gmail.com> escreveu:

On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela <ranier.vf@gmail.com>
wrote:

The problem is not only in nodeIncrementalSort.c, but in several others
too, where people are using TupIsNull with ExecCopySlot.
I would call this a design flaw.
If (TupIsNull)
ExecCopySlot,

The callers, think they are using TupIsNotNullAndEmpty.
If (TupIsNotNullAndEmpty)
ExecCopySlot

IMO both names are problematic, too data value centric, not semantic.
TupIsValid for the name and negating the existing tests would help to at
least clear that part up. Then, things operating on invalid tuples would
be expected to know about both representations. In the case of
ExecCopySlot there is nothing it can do with a null representation of an
invalid tuple so it would have to fail if presented one. An assertion
seems sufficient.

IHMO, assertion it is not the solution.

Steven suggested looking for some NULL pointer font above the calls.
I say that it is not necessary, there is no NULL pointer.
Whoever guarantees this is the combination, which for me is an assertion.

If (TupIsNull)
ExecCopySlot

It works as a subject, but in release mode.
It is the equivalent of:

If (TupIsNull)
Abort

The only problem for me is that we are running this assertion on the
clients' machines.

I cannot make heads nor tails of what you are trying to communicate here.

Ok. I will try to explain.

1. TupIsNull in fact it should be called: TupIsNullOrEmpty
2. Only Rename TupIsNull to example TupIsNullOrEmpty, improves, but it is
not the complete solution.
3. The combination:
if (TupIsNull(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);
for me it acts partly as if it were an assertion, but at runtime.
If node->group_pivot is NULL, ExecCopySlot crashes, like an assertion.
4. As it has been running for a while, without any complaints, probably the
callers have already guaranteed that node-> group_pivot is not NULL
5. We can remove the first part of the macro and rename: TupIsNull to
SlotEmpty
6. With SlotEmpty macro, each TupIsNull needs to be carefully changed.
if (SlotEmpty(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

I'll agree that TupIsNull isn't the most descriptive choice of name, and
is probably being abused throughout the code base, but the overall intent
and existing flow seems fine. My only goal would be to make it a bit
easier for unfamiliar coders to pick up on the coding pattern and
assumptions and make coding errors there more obvious. Renaming and/or an
assertion fits that goal. Breaking the current abstraction level doesn't
seem desirable.

If only rename TupIsNull to TupIsNullOrEmpty:

1. Why continue testing a pointer against NULL and call ExecCopySlot and
crash at runtime.
2. Most likely, the pointer is not NULL, since it has already been well
tested.
3. The only thing that can be done, after TupIsNullOrEmpty, is return or
fail, anything else needs to be tested again.

I think that current abstraction is broken.

regards,
Ranier Vilela

On Sun, Oct 11, 2020 at 6:27 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

Em dom., 11 de out. de 2020 às 14:53, David G. Johnston <
david.g.johnston@gmail.com> escreveu:

On Sun, Oct 11, 2020 at 3:31 AM Ranier Vilela <ranier.vf@gmail.com>
wrote:

Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston <
david.g.johnston@gmail.com> escreveu:

On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela <ranier.vf@gmail.com>
wrote:

The problem is not only in nodeIncrementalSort.c, but in several
others too, where people are using TupIsNull with ExecCopySlot.
I would call this a design flaw.
If (TupIsNull)
ExecCopySlot,

The callers, think they are using TupIsNotNullAndEmpty.
If (TupIsNotNullAndEmpty)
ExecCopySlot

IMO both names are problematic, too data value centric, not semantic.
TupIsValid for the name and negating the existing tests would help to at
least clear that part up. Then, things operating on invalid tuples would
be expected to know about both representations. In the case of
ExecCopySlot there is nothing it can do with a null representation of an
invalid tuple so it would have to fail if presented one. An assertion
seems sufficient.

IHMO, assertion it is not the solution.

Steven suggested looking for some NULL pointer font above the calls.
I say that it is not necessary, there is no NULL pointer.
Whoever guarantees this is the combination, which for me is an assertion.

If (TupIsNull)
ExecCopySlot

It works as a subject, but in release mode.
It is the equivalent of:

If (TupIsNull)
Abort

The only problem for me is that we are running this assertion on the
clients' machines.

I cannot make heads nor tails of what you are trying to communicate here.

Ok. I will try to explain.

1. TupIsNull in fact it should be called: TupIsNullOrEmpty
2. Only Rename TupIsNull to example TupIsNullOrEmpty, improves, but it is
not the complete solution.
3. The combination:
if (TupIsNull(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);
for me it acts partly as if it were an assertion, but at runtime.
If node->group_pivot is NULL, ExecCopySlot crashes, like an assertion.

Ok, but for me it's not an assertion, it's a higher-level check that the
variable that is expected to hold data on subsequent loops is, at the
beginning of the loop, uninitialized. TupIsUninitialized comes to mind as
better reflecting that fact.

4. As it has been running for a while, without any complaints, probably the

callers have already guaranteed that node-> group_pivot is not NULL

5. We can remove the first part of the macro and rename: TupIsNull to

SlotEmpty
6. With SlotEmpty macro, each TupIsNull needs to be carefully changed.
if (SlotEmpty(node->group_pivot))
ExecCopySlot(node->group_pivot, node->transfer_tuple);

I don't have a problem with introducing a SlotEmpty macro, and agree that
when it is followed by "ExecCopySlot" it is an meaningful improvement (the
blurring of the lines between a slot and its pointed-to-tuple bothers me as
I get my first exposure this to code).

I'll agree that TupIsNull isn't the most descriptive choice of name, and
is probably being abused throughout the code base, but the overall intent
and existing flow seems fine. My only goal would be to make it a bit
easier for unfamiliar coders to pick up on the coding pattern and
assumptions and make coding errors there more obvious. Renaming and/or an
assertion fits that goal. Breaking the current abstraction level doesn't
seem desirable.

If only rename TupIsNull to TupIsNullOrEmpty:

1. Why continue testing a pointer against NULL and call ExecCopySlot and
crash at runtime.
2. Most likely, the pointer is not NULL, since it has already been well
tested.
3. The only thing that can be done, after TupIsNullOrEmpty, is return or
fail, anything else needs to be tested again.

I think that current abstraction is broken.

I'm willing to agree that the abstraction is broken even if the end result
of its use, in the existing codebase, hasn't resulted in any known bugs
(again, the null pointer dereferencing seems like it should be picked up
during routine testing). That said, there are multiple solutions here that
would improve matters in varying degrees each having a proportional effort
and risk profile in writing a patch (including the status-quo option).

For me, while I see room for improvement here, my total lack of actually
writing code using these interfaces means I defer to Tom Lane's final two
conclusions in his last email regarding how productive this line of work
would be. I also read that to mean if there was a complete and thorough
patch submitted it would be given a fair look. I would hope so since there
is a meaningful decision to make with regards to making changes purely to
benefit future inexperienced coders. But it seems like worthy material for
an inexperienced coder to compile and present and having the experienced
coders evaluate and critique, as Stephen Frost's post seemed to allude to.

David J.