Mop up some undue familiarity with the innards of Bitmapsets.

nodeAppend.c used non-nullness of appendstate->as_valid_subplans as
a state flag to indicate whether it'd done ExecFindMatchingSubPlans
(or some sufficient approximation to that).  This was pretty
questionable even in the beginning, since it wouldn't really work
right if there are no valid subplans.  It got more questionable
after commit 27e1f1456 added logic that could reduce as_valid_subplans
to an empty set: at that point we were depending on unspecified
behavior of bms_del_members, namely that it'd not return an empty
set as NULL.  It's about to start doing that, which breaks this
logic entirely.  Hence, add a separate boolean flag to signal
whether as_valid_subplans has been computed.

Also fix a previously-cosmetic bug in nodeAgg.c, wherein it ignored
the return value of bms_del_member instead of updating its pointer.

Patch by me; thanks to Nathan Bossart and Richard Guo for review.

Discussion: https://postgr.es/m/1159933.1677621588@sss.pgh.pa.us

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 5960e4a6c19a2dae296688926cf56a4efc0e4521..19342a420c10b0add0b68db1bddf411d53983d12 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1642,7 +1642,7 @@ find_hash_columns(AggState *aggstate)
                        perhash->hashGrpColIdxHash[i] = i + 1;
                        perhash->numhashGrpCols++;
                        /* delete already mapped columns */
-                       bms_del_member(colnos, grpColIdx[i]);
+                       colnos = bms_del_member(colnos, grpColIdx[i]);
                }
 
                /* and add the remaining columns */

diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index cb25499b3f23ece40c1669b8b06f2778758a1b34..c185b11c6711b0b86832184c499c8a51bae599cf 100644 (file)
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -157,7 +157,10 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
                 * later calls to ExecFindMatchingSubPlans.
                 */
                if (!prunestate->do_exec_prune && nplans > 0)
+               {
                        appendstate->as_valid_subplans = bms_add_range(NULL, 0, nplans - 1);
+                       appendstate->as_valid_subplans_identified = true;
+               }
        }
        else
        {
@@ -170,6 +173,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
                Assert(nplans > 0);
                appendstate->as_valid_subplans = validsubplans =
                        bms_add_range(NULL, 0, nplans - 1);
+               appendstate->as_valid_subplans_identified = true;
                appendstate->as_prune_state = NULL;
        }
 
@@ -259,7 +263,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
                appendstate->as_asyncresults = (TupleTableSlot **)
                        palloc0(nasyncplans * sizeof(TupleTableSlot *));
 
-               if (appendstate->as_valid_subplans != NULL)
+               if (appendstate->as_valid_subplans_identified)
                        classify_matching_subplans(appendstate);
        }
 
@@ -414,13 +418,11 @@ ExecReScanAppend(AppendState *node)
                bms_overlap(node->ps.chgParam,
                                        node->as_prune_state->execparamids))
        {
+               node->as_valid_subplans_identified = false;
                bms_free(node->as_valid_subplans);
                node->as_valid_subplans = NULL;
-               if (nasyncplans > 0)
-               {
-                       bms_free(node->as_valid_asyncplans);
-                       node->as_valid_asyncplans = NULL;
-               }
+               bms_free(node->as_valid_asyncplans);
+               node->as_valid_asyncplans = NULL;
        }
 
        for (i = 0; i < node->as_nplans; i++)
@@ -574,11 +576,14 @@ choose_next_subplan_locally(AppendState *node)
                if (node->as_nasyncplans > 0)
                {
                        /* We'd have filled as_valid_subplans already */
-                       Assert(node->as_valid_subplans);
+                       Assert(node->as_valid_subplans_identified);
                }
-               else if (node->as_valid_subplans == NULL)
+               else if (!node->as_valid_subplans_identified)
+               {
                        node->as_valid_subplans =
                                ExecFindMatchingSubPlans(node->as_prune_state, false);
+                       node->as_valid_subplans_identified = true;
+               }
 
                whichplan = -1;
        }
@@ -640,10 +645,11 @@ choose_next_subplan_for_leader(AppendState *node)
                 * run-time pruning is disabled then the valid subplans will always be
                 * set to all subplans.
                 */
-               if (node->as_valid_subplans == NULL)
+               if (!node->as_valid_subplans_identified)
                {
                        node->as_valid_subplans =
                                ExecFindMatchingSubPlans(node->as_prune_state, false);
+                       node->as_valid_subplans_identified = true;
 
                        /*
                         * Mark each invalid plan as finished to allow the loop below to
@@ -715,10 +721,12 @@ choose_next_subplan_for_worker(AppendState *node)
         * run-time pruning is disabled then the valid subplans will always be set
         * to all subplans.
         */
-       else if (node->as_valid_subplans == NULL)
+       else if (!node->as_valid_subplans_identified)
        {
                node->as_valid_subplans =
                        ExecFindMatchingSubPlans(node->as_prune_state, false);
+               node->as_valid_subplans_identified = true;
+
                mark_invalid_subplans_as_finished(node);
        }
 
@@ -866,10 +874,11 @@ ExecAppendAsyncBegin(AppendState *node)
        Assert(node->as_nasyncplans > 0);
 
        /* If we've yet to determine the valid subplans then do so now. */
-       if (node->as_valid_subplans == NULL)
+       if (!node->as_valid_subplans_identified)
        {
                node->as_valid_subplans =
                        ExecFindMatchingSubPlans(node->as_prune_state, false);
+               node->as_valid_subplans_identified = true;
 
                classify_matching_subplans(node);
        }
@@ -1143,6 +1152,7 @@ classify_matching_subplans(AppendState *node)
 {
        Bitmapset  *valid_asyncplans;
 
+       Assert(node->as_valid_subplans_identified);
        Assert(node->as_valid_asyncplans == NULL);
 
        /* Nothing to do if there are no valid subplans. */
@@ -1161,9 +1171,8 @@ classify_matching_subplans(AppendState *node)
        }
 
        /* Get valid async subplans. */
-       valid_asyncplans = bms_copy(node->as_asyncplans);
-       valid_asyncplans = bms_int_members(valid_asyncplans,
-                                                                          node->as_valid_subplans);
+       valid_asyncplans = bms_intersect(node->as_asyncplans,
+                                                                        node->as_valid_subplans);
 
        /* Adjust the valid subplans to contain sync subplans only. */
        node->as_valid_subplans = bms_del_members(node->as_valid_subplans,

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 20f4c8b35f31b8977c0ed069b94f94734f9a89b7..e7eb1e666ffa0bd155f383e2f6fbed6f498996c7 100644 (file)
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1350,6 +1350,7 @@ struct AppendState
        ParallelAppendState *as_pstate; /* parallel coordination info */
        Size            pstate_len;             /* size of parallel coordination info */
        struct PartitionPruneState *as_prune_state;
+       bool            as_valid_subplans_identified;   /* is as_valid_subplans valid? */
        Bitmapset  *as_valid_subplans;
        Bitmapset  *as_valid_asyncplans;        /* valid asynchronous plans indexes */
        bool            (*choose_next_subplan) (AppendState *);