Re: Check for tuplestorestate nullness before dereferencing

Nikolay Shaplov <dhyan(at)nataraj(dot)su> writes:
> В письме от пятница, 18 октября 2024 г. 02:28:22 MSK пользователь David Rowley
> написал:

>> It would be good to know if the optimisation added in d2c555ee5 ever
>> applies with today's code. If it does apply, we should likely add a
>> test case for it and if it never does, then we should just remove the
>> optimisation and always create the tuplestore when it's NULL.

> That's sounds reasonable. It looks like that removing "node->eflags != 0" check
> is more logical then adding not null check.

I don't believe that the patch as-proposed is necessary or reasonable.

The case of node->eflags == 0 is reachable; I don't know why David's
test didn't see this, but when I try asserting the contrary I get
a regression crash on

(gdb) p debug_query_string
$1 = 0x1d03160 "explain (costs off) declare c1 scroll cursor for select (select 42) as x;"

The reason that the subsequent bit of code is safe is that !forward
should not possibly be true unless EXEC_FLAG_BACKWARD was given,
which'd cause us to create a tuplestore. So if we were going
to change anything, I'd propose adding something more like

if (!forward && eof_tuplestore)
{
+ Assert(node->eflags & EXEC_FLAG_BACKWARD);
if (!node->eof_underlying)
{
/*

or perhaps more directly, Assert that tuplestore is not null.
But I don't like the proposed patch because if tuplestore is
null here, something's wrong, and we should complain not
silently mask the problem.

regards, tom lane