Re: Check for tuplestorestate nullness before dereferencing

14.10.2024 19:21, Alena Rybakina wrote:
> To be honest, I'm not sure this change is necessary. Looking at the code, I see that in ExecMaterial it is possible to handle a tuplestorestate of NULL, and this error can be accessed if the flags are not zero, but I think these cases have been worked out.
> [...]
> After the subplan scan is complete, we see that the eof_underlying variable becomes true, and this part of the code will no longer be accessible. tuplestorestate also becomes Null.
>
> I also noticed that tuplestorestate=NULL is an indicator that the scan is complete, so if this section of code is called, something is wrong earlier in the code.
It appears to me that prior to the earlier commit [1], tuplestorestate was
always initialized if it was NULL. In that commit, flags were introduced,
allowing for the possibility that tuplestorestate could remain NULL further
along in the code. This is why I believe that checks for its nullness were
added before calling dereferencing functions, such as the check in line 146,
which is still present.

However, the tuplestore_getheaptuple() function, which is no longer present,
remained unchanged until commit [2], where it was replaced with
tuplestore_gettupleslot() and tuplestore_advance(). While it was acceptable in
case of tuplestore_gettupleslot() that can handle a NULL tuplestorestate,
tuplestore_advance() requires tuplestorestate to not be NULL when it is called.

This leads to my confusion as to why there is no check for the nullness of
tuplestorestate before calling tuplestore_advance().

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d2c555ee538f34be7aff744b994df4d2369a9140
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3f50ba27cf417eb57fd310c2a88f76a6ea6b966e

--
Best regards,
Alexander Kuznetsov