I noticed a definitional problem in 0001 that's also a bug in some
conditions -- namely that the bitmapset "validplans" is never explicitly
initialized to NIL. In the original coding, the BMS was always returned
from somewhere; in the new code, it is passed from an uninitialized
stack variable into the new ExecInitPartitionPruning function, which
then proceeds to add new members to it without initializing it first.
Indeed that function's header comment explicitly indicates that it is
not initialized:
+ * Initial pruning can be done immediately, so it is done here if needed and
+ * the set of surviving partition subplans' indexes are added to the output
+ * parameter *initially_valid_subplans.
even though this is not fully correct, because when prunestate->do_initial_prune
is false, then the BMS *is* initialized.
I have no opinion on where to initialize it, but it needs to be done
somewhere and the comment needs to agree.
I think the names ExecCreatePartitionPruneState and
ExecInitPartitionPruning are too confusingly similar. Maybe the former
should be renamed to somehow make it clear that it is a subroutine for
the former.
At the top of the file, there's a new comment that reads:
* ExecInitPartitionPruning:
* Creates the PartitionPruneState required by each of the two pruning
* functions.
What are "the two pruning functions"? I think here you mean "Append"
and "MergeAppend". Maybe spell that out explicitly.
I think this comment needs to be reworded:
+ * Subplans would previously be indexed 0..(n_total_subplans - 1) should be
+ * changed to index range 0..num(initially_valid_subplans).
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
