Per the discussion at [1], it seems like it'd be a good idea to make
Bitmapsets into full-fledged, tagged Nodes, so that we could do things
like print or copy lists of them without special-case logic. The
extra space for the NodeTag is basically free due to alignment
considerations, at least on 64-bit hardware.
Attached is a cleaned-up version of Amit's patch v24-0003 at [2].
I fixed the problems with not always tagging Bitmapsets, and changed
the outfuncs/readfuncs logic so that Bitmapsets still print exactly
as they did before (thus, this doesn't require a catversion bump).
As proof of concept, I removed the read_write_ignore labels from
RelOptInfo's unique_for_rels and non_unique_for_rels fields, and
got nice-looking debug printout:
:unique_for_rels ((b 1))
:non_unique_for_rels <>
I also removed some special-case code from indxpath.c because
list_member() can do the same thing now. (There might be other
places that can be simplified; I didn't look very hard.)
It'd be possible to make Bitmapset fields be (mostly) not special cases
in the copy/equal/out/read support. But I chose to leave that alone,
because it'd add a little runtime overhead for indirecting through the
generic support functions while not really saving any code space.
Barring objections, I'd like to go ahead and push this.
regards, tom lane
[1]
https://www.postgresql.org/message-id/94353655-c177-1f55-7afb-b2090de33341%40enterprisedb.com
[2]
https://www.postgresql.org/message-id/CA%2BHiwqEYCLRZ2Boq_uK0pjLn_9b8XL-LmwKj7HN5kJOivUkYLg%40mail.gmail.com
diff --git a/src/backend/nodes/Makefile b/src/backend/nodes/Makefile
index 7450e191ee..d7b4261b47 100644
--- a/src/backend/nodes/Makefile
+++ b/src/backend/nodes/Makefile
@@ -52,6 +52,7 @@ node_headers = \
commands/trigger.h \
executor/tuptable.h \
foreign/fdwapi.h \
+ nodes/bitmapset.h \
nodes/extensible.h \
nodes/lockoptions.h \
nodes/replnodes.h \
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index 0a6c30e4eb..b7b274aeff 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -194,6 +194,7 @@ bms_make_singleton(int x)
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
result = (Bitmapset *) palloc0(BITMAPSET_SIZE(wordnum + 1));
+ result->type = T_Bitmapset;
result->nwords = wordnum + 1;
result->words[wordnum] = ((bitmapword) 1 << bitnum);
return result;
@@ -852,6 +853,7 @@ bms_add_range(Bitmapset *a, int lower, int upper)
if (a == NULL)
{
a = (Bitmapset *) palloc0(BITMAPSET_SIZE(uwordnum + 1));
+ a->type = T_Bitmapset;
a->nwords = uwordnum + 1;
}
else if (uwordnum >= a->nwords)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index e76fda8eba..84f5e2e6ad 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -160,6 +160,12 @@ _copyExtensibleNode(const ExtensibleNode *from)
return newnode;
}
+static Bitmapset *
+_copyBitmapset(const Bitmapset *from)
+{
+ return bms_copy(from);
+}
+
/*
* copyObjectImpl -- implementation of copyObject(); see nodes/nodes.h
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 0373aa30fe..b2f07da62e 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -145,6 +145,12 @@ _equalA_Const(const A_Const *a, const A_Const *b)
return true;
}
+static bool
+_equalBitmapset(const Bitmapset *a, const Bitmapset *b)
+{
+ return bms_equal(a, b);
+}
+
/*
* Lists are handled specially
*/
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 81b8c184a9..d3f25299de 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -65,6 +65,7 @@ my @all_input_files = qw(
commands/trigger.h
executor/tuptable.h
foreign/fdwapi.h
+ nodes/bitmapset.h
nodes/extensible.h
nodes/lockoptions.h
nodes/replnodes.h
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 64c65f060b..f05e72f0dc 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -314,6 +314,9 @@ _outList(StringInfo str, const List *node)
* converts a bitmap set of integers
*
* Note: the output format is "(b int int ...)", similar to an integer List.
+ *
+ * We export this function for use by extensions that define extensible nodes.
+ * That's somewhat historical, though, because calling outNode() will work.
*/
void
outBitmapset(StringInfo str, const Bitmapset *bms)
@@ -844,6 +847,8 @@ outNode(StringInfo str, const void *obj)
_outString(str, (String *) obj);
else if (IsA(obj, BitString))
_outBitString(str, (BitString *) obj);
+ else if (IsA(obj, Bitmapset))
+ outBitmapset(str, (Bitmapset *) obj);
else
{
appendStringInfoChar(str, '{');
diff --git a/src/backend/nodes/read.c b/src/backend/nodes/read.c
index fe84f140ee..74d9d754d3 100644
--- a/src/backend/nodes/read.c
+++ b/src/backend/nodes/read.c
@@ -22,6 +22,7 @@
#include <ctype.h>
#include "common/string.h"
+#include "nodes/bitmapset.h"
#include "nodes/pg_list.h"
#include "nodes/readfuncs.h"
#include "nodes/value.h"
@@ -347,6 +348,7 @@ nodeRead(const char *token, int tok_len)
* Could be an integer list: (i int int ...)
* or an OID list: (o int int ...)
* or an XID list: (x int int ...)
+ * or a bitmapset: (b int int ...)
* or a list of nodes/values: (node node ...)
*----------
*/
@@ -372,6 +374,7 @@ nodeRead(const char *token, int tok_len)
tok_len, token);
l = lappend_int(l, val);
}
+ result = (Node *) l;
}
else if (tok_len == 1 && token[0] == 'o')
{
@@ -392,6 +395,7 @@ nodeRead(const char *token, int tok_len)
tok_len, token);
l = lappend_oid(l, val);
}
+ result = (Node *) l;
}
else if (tok_len == 1 && token[0] == 'x')
{
@@ -412,6 +416,30 @@ nodeRead(const char *token, int tok_len)
tok_len, token);
l = lappend_xid(l, val);
}
+ result = (Node *) l;
+ }
+ else if (tok_len == 1 && token[0] == 'b')
+ {
+ /* Bitmapset -- see also _readBitmapset() */
+ Bitmapset *bms = NULL;
+
+ for (;;)
+ {
+ int val;
+ char *endptr;
+
+ token = pg_strtok(&tok_len);
+ if (token == NULL)
+ elog(ERROR, "unterminated Bitmapset structure");
+ if (tok_len == 1 && token[0] == ')')
+ break;
+ val = (int) strtol(token, &endptr, 10);
+ if (endptr != token + tok_len)
+ elog(ERROR, "unrecognized integer: \"%.*s\"",
+ tok_len, token);
+ bms = bms_add_member(bms, val);
+ }
+ result = (Node *) bms;
}
else
{
@@ -426,8 +454,8 @@ nodeRead(const char *token, int tok_len)
if (token == NULL)
elog(ERROR, "unterminated List structure");
}
+ result = (Node *) l;
}
- result = (Node *) l;
break;
}
case RIGHT_PAREN:
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index b4ff855f7c..23776367c5 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -194,6 +194,10 @@ nullable_string(const char *token, int length)
/*
* _readBitmapset
+ *
+ * Note: this code is used in contexts where we know that a Bitmapset
+ * is expected. There is equivalent code in nodeRead() that can read a
+ * Bitmapset when we come across one in other contexts.
*/
static Bitmapset *
_readBitmapset(void)
@@ -234,7 +238,8 @@ _readBitmapset(void)
}
/*
- * for use by extensions which define extensible nodes
+ * We export this function for use by extensions that define extensible nodes.
+ * That's somewhat historical, though, because calling nodeRead() will work.
*/
Bitmapset *
readBitmapset(void)
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 77f3f81bcb..914bfd90bc 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -99,7 +99,6 @@ static void get_join_index_paths(PlannerInfo *root, RelOptInfo *rel,
List **considered_relids);
static bool eclass_already_used(EquivalenceClass *parent_ec, Relids oldrelids,
List *indexjoinclauses);
-static bool bms_equal_any(Relids relids, List *relids_list);
static void get_index_paths(PlannerInfo *root, RelOptInfo *rel,
IndexOptInfo *index, IndexClauseSet *clauses,
List **bitindexpaths);
@@ -370,8 +369,8 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
Path *path = (Path *) lfirst(lc);
Relids required_outer = PATH_REQ_OUTER(path);
- if (!bms_equal_any(required_outer, all_path_outers))
- all_path_outers = lappend(all_path_outers, required_outer);
+ all_path_outers = list_append_unique(all_path_outers,
+ required_outer);
}
/* Now, for each distinct parameterization set ... */
@@ -517,7 +516,7 @@ consider_index_join_outer_rels(PlannerInfo *root, RelOptInfo *rel,
int num_considered_relids;
/* If we already tried its relids set, no need to do so again */
- if (bms_equal_any(clause_relids, *considered_relids))
+ if (list_member(*considered_relids, clause_relids))
continue;
/*
@@ -612,7 +611,7 @@ get_join_index_paths(PlannerInfo *root, RelOptInfo *rel,
int indexcol;
/* If we already considered this relids set, don't repeat the work */
- if (bms_equal_any(relids, *considered_relids))
+ if (list_member(*considered_relids, relids))
return;
/* Identify indexclauses usable with this relids set */
@@ -694,25 +693,6 @@ eclass_already_used(EquivalenceClass *parent_ec, Relids oldrelids,
return false;
}
-/*
- * bms_equal_any
- * True if relids is bms_equal to any member of relids_list
- *
- * Perhaps this should be in bitmapset.c someday.
- */
-static bool
-bms_equal_any(Relids relids, List *relids_list)
-{
- ListCell *lc;
-
- foreach(lc, relids_list)
- {
- if (bms_equal(relids, (Relids) lfirst(lc)))
- return true;
- }
- return false;
-}
-
/*
* get_index_paths
diff --git a/src/include/nodes/bitmapset.h b/src/include/nodes/bitmapset.h
index 75b5ce1a8e..2792281658 100644
--- a/src/include/nodes/bitmapset.h
+++ b/src/include/nodes/bitmapset.h
@@ -20,6 +20,8 @@
#ifndef BITMAPSET_H
#define BITMAPSET_H
+#include "nodes/nodes.h"
+
/*
* Forward decl to save including pg_list.h
*/
@@ -48,6 +50,9 @@ typedef int32 signedbitmapword; /* must be the matching signed type */
typedef struct Bitmapset
{
+ pg_node_attr(custom_copy_equal, special_read_write)
+
+ NodeTag type;
int nwords; /* number of words in array */
bitmapword words[FLEXIBLE_ARRAY_MEMBER]; /* really [nwords] */
} Bitmapset;
diff --git a/src/include/nodes/meson.build b/src/include/nodes/meson.build
index b7df232081..e63881086e 100644
--- a/src/include/nodes/meson.build
+++ b/src/include/nodes/meson.build
@@ -13,6 +13,7 @@ node_support_input_i = [
'commands/trigger.h',
'executor/tuptable.h',
'foreign/fdwapi.h',
+ 'nodes/bitmapset.h',
'nodes/extensible.h',
'nodes/lockoptions.h',
'nodes/replnodes.h',
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 09342d128d..a544b313d3 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -911,13 +911,11 @@ typedef struct RelOptInfo
/*
* cache space for remembering if we have proven this relation unique
- *
- * can't print unique_for_rels/non_unique_for_rels; BMSes aren't Nodes
*/
/* known unique for these other relid set(s) */
- List *unique_for_rels pg_node_attr(read_write_ignore);
+ List *unique_for_rels;
/* known not unique for these set(s) */
- List *non_unique_for_rels pg_node_attr(read_write_ignore);
+ List *non_unique_for_rels;
/*
* used by various scans and joins:
