Remove bms_first_member().

This function has been semi-deprecated ever since we invented
bms_next_member().  Its habit of scribbling on the input bitmapset
isn't great, plus for sufficiently large bitmapsets it would take
O(N^2) time to complete a loop.  Now we have the additional problem
that reducing the input to empty while leaving it still accessible
would violate a planned invariant.  So let's just get rid of it,
after updating the few extant callers to use bms_next_member().

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/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 8ccc1675488eeade58dc3847d33b9a29c1718332..2d2b0b6a6b91c24cad626259d9c902bbbb24dffa 100644 (file)
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -858,7 +858,7 @@ check_selective_binary_conversion(RelOptInfo *baserel,
        ListCell   *lc;
        Relation        rel;
        TupleDesc       tupleDesc;
-       AttrNumber      attnum;
+       int                     attidx;
        Bitmapset  *attrs_used = NULL;
        bool            has_wholerow = false;
        int                     numattrs;
@@ -901,10 +901,11 @@ check_selective_binary_conversion(RelOptInfo *baserel,
        rel = table_open(foreigntableid, AccessShareLock);
        tupleDesc = RelationGetDescr(rel);
 
-       while ((attnum = bms_first_member(attrs_used)) >= 0)
+       attidx = -1;
+       while ((attidx = bms_next_member(attrs_used, attidx)) >= 0)
        {
-               /* Adjust for system attributes. */
-               attnum += FirstLowInvalidHeapAttributeNumber;
+               /* attidx is zero-based, attnum is the normal attribute number */
+               AttrNumber      attnum = attidx + FirstLowInvalidHeapAttributeNumber;
 
                if (attnum == 0)
                {

diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c
index 3cc927c79ed76c7bfcc5f6f1a3a4de209ea5794b..8c8f6f1e3a115b5b7926160e4ad46a9953350b8e 100644 (file)
--- a/contrib/sepgsql/dml.c
+++ b/contrib/sepgsql/dml.c
@@ -231,7 +231,8 @@ check_relation_privileges(Oid relOid,
        updated = fixup_whole_row_references(relOid, updated);
        columns = bms_union(selected, bms_union(inserted, updated));
 
-       while ((index = bms_first_member(columns)) >= 0)
+       index = -1;
+       while ((index = bms_next_member(columns, index)) >= 0)
        {
                AttrNumber      attnum;
                uint32          column_perms = 0;

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 7eb79cee58d6b26411dd6c388641242d63f33682..4f50e0dd3478d89defe4a2d18ce8bc3d2e142c2c 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3859,9 +3859,6 @@ heap_attr_equals(TupleDesc tupdesc, int attrnum, Datum value1, Datum value2,
  * has_external indicates if any of the unmodified attributes (from those
  * listed as interesting) of the old tuple is a member of external_cols and is
  * stored externally.
- *
- * The input interesting_cols bitmapset is destructively modified; that is OK
- * since this is invoked at most once in heap_update.
  */
 static Bitmapset *
 HeapDetermineColumnsInfo(Relation relation,
@@ -3870,19 +3867,20 @@ HeapDetermineColumnsInfo(Relation relation,
                                                 HeapTuple oldtup, HeapTuple newtup,
                                                 bool *has_external)
 {
-       int                     attrnum;
+       int                     attidx;
        Bitmapset  *modified = NULL;
        TupleDesc       tupdesc = RelationGetDescr(relation);
 
-       while ((attrnum = bms_first_member(interesting_cols)) >= 0)
+       attidx = -1;
+       while ((attidx = bms_next_member(interesting_cols, attidx)) >= 0)
        {
+               /* attidx is zero-based, attrnum is the normal attribute number */
+               AttrNumber      attrnum = attidx + FirstLowInvalidHeapAttributeNumber;
                Datum           value1,
                                        value2;
                bool            isnull1,
                                        isnull2;
 
-               attrnum += FirstLowInvalidHeapAttributeNumber;
-
                /*
                 * If it's a whole-tuple reference, say "not equal".  It's not really
                 * worth supporting this case, since it could only succeed after a
@@ -3890,9 +3888,7 @@ HeapDetermineColumnsInfo(Relation relation,
                 */
                if (attrnum == 0)
                {
-                       modified = bms_add_member(modified,
-                                                                         attrnum -
-                                                                         FirstLowInvalidHeapAttributeNumber);
+                       modified = bms_add_member(modified, attidx);
                        continue;
                }
 
@@ -3905,9 +3901,7 @@ HeapDetermineColumnsInfo(Relation relation,
                {
                        if (attrnum != TableOidAttributeNumber)
                        {
-                               modified = bms_add_member(modified,
-                                                                                 attrnum -
-                                                                                 FirstLowInvalidHeapAttributeNumber);
+                               modified = bms_add_member(modified, attidx);
                                continue;
                        }
                }
@@ -3924,9 +3918,7 @@ HeapDetermineColumnsInfo(Relation relation,
                if (!heap_attr_equals(tupdesc, attrnum, value1,
                                                          value2, isnull1, isnull2))
                {
-                       modified = bms_add_member(modified,
-                                                                         attrnum -
-                                                                         FirstLowInvalidHeapAttributeNumber);
+                       modified = bms_add_member(modified, attidx);
                        continue;
                }
 
@@ -3943,8 +3935,7 @@ HeapDetermineColumnsInfo(Relation relation,
                 * member of external_cols.
                 */
                if (VARATT_IS_EXTERNAL((struct varlena *) DatumGetPointer(value1)) &&
-                       bms_is_member(attrnum - FirstLowInvalidHeapAttributeNumber,
-                                                 external_cols))
+                       bms_is_member(attidx, external_cols))
                        *has_external = true;
        }
 

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 20d23696a5357247b90cdfc20b15fa5b418ea001..5960e4a6c19a2dae296688926cf56a4efc0e4521 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1646,7 +1646,8 @@ find_hash_columns(AggState *aggstate)
                }
 
                /* and add the remaining columns */
-               while ((i = bms_first_member(colnos)) >= 0)
+               i = -1;
+               while ((i = bms_next_member(colnos, i)) >= 0)
                {
                        perhash->hashGrpColIdxInput[perhash->numhashGrpCols] = i;
                        perhash->numhashGrpCols++;

diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index 98660524adefe4362832e941c2169b464c817a1f..10dbbdddf58a32d2b987c2efd1b3e6d5e0c0932a 100644 (file)
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -982,48 +982,6 @@ bms_join(Bitmapset *a, Bitmapset *b)
        return result;
 }
 
-/*
- * bms_first_member - find and remove first member of a set
- *
- * Returns -1 if set is empty.  NB: set is destructively modified!
- *
- * This is intended as support for iterating through the members of a set.
- * The typical pattern is
- *
- *                     while ((x = bms_first_member(inputset)) >= 0)
- *                             process member x;
- *
- * CAUTION: this destroys the content of "inputset".  If the set must
- * not be modified, use bms_next_member instead.
- */
-int
-bms_first_member(Bitmapset *a)
-{
-       int                     nwords;
-       int                     wordnum;
-
-       if (a == NULL)
-               return -1;
-       nwords = a->nwords;
-       for (wordnum = 0; wordnum < nwords; wordnum++)
-       {
-               bitmapword      w = a->words[wordnum];
-
-               if (w != 0)
-               {
-                       int                     result;
-
-                       w = RIGHTMOST_ONE(w);
-                       a->words[wordnum] &= ~w;
-
-                       result = wordnum * BITS_PER_BITMAPWORD;
-                       result += bmw_rightmost_one_pos(w);
-                       return result;
-               }
-       }
-       return -1;
-}
-
 /*
  * bms_next_member - find next member of a set
  *

diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 04eda4798efeab219711b81c5c127f5375321cf4..22ffe4ca425af29d4afa9f404acdf9e5b7407a3c 100644 (file)
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -1481,7 +1481,8 @@ convert_EXISTS_sublink_to_join(PlannerInfo *root, SubLink *sublink,
         */
        clause_varnos = pull_varnos(root, whereClause);
        upper_varnos = NULL;
-       while ((varno = bms_first_member(clause_varnos)) >= 0)
+       varno = -1;
+       while ((varno = bms_next_member(clause_varnos, varno)) >= 0)
        {
                if (varno <= rtoffset)
                        upper_varnos = bms_add_member(upper_varnos, varno);

diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 7ff41acb846315d9d405f862b2d91ac528df1735..78221d2e0f7a863d60d2d664faa3d69d48190688 100644 (file)
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2759,7 +2759,7 @@ make_row_comparison_op(ParseState *pstate, List *opname,
         * them ... this coding arbitrarily picks the lowest btree strategy
         * number.
         */
-       i = bms_first_member(strats);
+       i = bms_next_member(strats, -1);
        if (i < 0)
        {
                /* No common interpretation, so fail */

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 9f139c64dba7ad4865688326e58f64f5b800d93d..55bfa078711529a1476f1c6ba881b4d9cdab8efc 100644 (file)
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -227,7 +227,8 @@ logicalrep_report_missing_attrs(LogicalRepRelation *remoterel,
 
                initStringInfo(&missingattsbuf);
 
-               while ((i = bms_first_member(missingatts)) >= 0)
+               i = -1;
+               while ((i = bms_next_member(missingatts, i)) >= 0)
                {
                        missingattcnt++;
                        if (missingattcnt == 1)

diff --git a/src/include/nodes/bitmapset.h b/src/include/nodes/bitmapset.h
index 3d2225e1ae05240945ed351beb6f499b1fe06218..c344ac04bec1e50fe39996bc240b35b185be7dd0 100644 (file)
--- a/src/include/nodes/bitmapset.h
+++ b/src/include/nodes/bitmapset.h
@@ -115,7 +115,6 @@ extern Bitmapset *bms_del_members(Bitmapset *a, const Bitmapset *b);
 extern Bitmapset *bms_join(Bitmapset *a, Bitmapset *b);
 
 /* support for iterating through the integer elements of a set: */
-extern int     bms_first_member(Bitmapset *a);
 extern int     bms_next_member(const Bitmapset *a, int prevbit);
 extern int     bms_prev_member(const Bitmapset *a, int prevbit);