agora inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 8/8] Consider selectivity in choose_bitmap_and
3+ messages / 2 participants
[nested] [flat]

* [PATCH 8/8] Consider selectivity in choose_bitmap_and
@ 2021-03-11 11:57  Tomas Vondra <[email protected]>
  0 siblings, 0 replies; 3+ messages in thread

From: Tomas Vondra @ 2021-03-11 11:57 UTC (permalink / raw)

---
 src/backend/optimizer/path/indxpath.c | 38 ++++++++++++++++-----------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index ff536e6b24..36b29c6c7b 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -1437,6 +1437,7 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel, List *paths)
 	foreach(l, paths)
 	{
 		Path	   *ipath = (Path *) lfirst(l);
+		bool		duplicate = false;
 
 		pathinfo = classify_index_clause_usage(ipath, &clauselist);
 
@@ -1451,22 +1452,29 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel, List *paths)
 		{
 			if (!pathinfoarray[i]->unclassifiable &&
 				bms_equal(pathinfo->clauseids, pathinfoarray[i]->clauseids))
-				break;
-		}
-		if (i < npaths)
-		{
-			/* duplicate clauseids, keep the cheaper one */
-			Cost		ncost;
-			Cost		ocost;
-			Selectivity nselec;
-			Selectivity oselec;
-
-			cost_bitmap_tree_node(pathinfo->path, &ncost, &nselec);
-			cost_bitmap_tree_node(pathinfoarray[i]->path, &ocost, &oselec);
-			if (ncost < ocost)
-				pathinfoarray[i] = pathinfo;
+			{
+				/* duplicate clauseids, keep the cheaper one
+				 *
+				 * XXX maybe this should just use path_usage_comparator?
+				 */
+				Cost		ncost;
+				Cost		ocost;
+				Selectivity nselec;
+				Selectivity oselec;
+
+				cost_bitmap_tree_node(pathinfo->path, &ncost, &nselec);
+				cost_bitmap_tree_node(pathinfoarray[i]->path, &ocost, &oselec);
+
+				if ((ncost < ocost) && (nselec < oselec))
+				{
+					pathinfoarray[i] = pathinfo;
+					duplicate = true;
+					break;
+				}
+			}
 		}
-		else
+
+		if (!duplicate)
 		{
 			/* not duplicate clauseids, add to array */
 			pathinfoarray[npaths++] = pathinfo;
-- 
2.26.2


--------------D524CD37E77B36A2883A7617
Content-Type: application/sql;
 name="reproduce2.sql"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="reproduce2.sql"

ZHJvcCB0YWJsZSBpZiBleGlzdHMgdDsKCnNldCBtYXhfcGFyYWxsZWxfd29ya2Vyc19wZXJf
Z2F0aGVyID0gMDsKCmNyZWF0ZSB0YWJsZSB0KGEgYmlnaW50KSB3aXRoIChmaWxsZmFjdG9y
ID0gMjUpOwoKaW5zZXJ0IGludG8gdCBzZWxlY3QgaSBmcm9tIGdlbmVyYXRlX3Nlcmllcygx
LDEwMDAwMDAwKSBzKGkpOwoKY3JlYXRlIGluZGV4IGlkeF8xIG9uIHQgdXNpbmcgYnJpbiAo
YSkgd2l0aCAocGFnZXNfcGVyX3JhbmdlID0gNCk7CgpjcmVhdGUgaW5kZXggaWR4XzIgb24g
dCB1c2luZyBicmluIChhKTsKCmFuYWx5emUgdDsKClxkKwpcZGkrCgpleHBsYWluIGFuYWx5
emUgc2VsZWN0ICogZnJvbSB0IHdoZXJlIGEgPSAxMDAwOwoKZHJvcCBpbmRleCBpZHhfMjsK
CmV4cGxhaW4gYW5hbHl6ZSBzZWxlY3QgKiBmcm9tIHQgd2hlcmUgYSA9IDEwMDA7Cg==
--------------D524CD37E77B36A2883A7617
Content-Type: application/sql;
 name="reproducer.sql"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="reproducer.sql"

ZHJvcCB0YWJsZSBpZiBleGlzdHMgdDsKCnNldCBtYXhfcGFyYWxsZWxfd29ya2Vyc19wZXJf
Z2F0aGVyID0gMDsKCmNyZWF0ZSB0YWJsZSB0IChhIGludCwgYiBiaWdpbnQsIGMgdGV4dCkg
d2l0aCAoZmlsbGZhY3RvciA9IDUwKTsKCmluc2VydCBpbnRvIHQgc2VsZWN0IDEwMDAwMDAg
KiByYW5kb20oKSwgMTAwMDAwMCAqIHJhbmRvbSgpLCBtZDUoKDEwMDAwMDAgKiByYW5kb20o
KSk6OmludDo6dGV4dCkgZnJvbSBnZW5lcmF0ZV9zZXJpZXMoMSwxMDAwMDAwKSBzKGkpOwoK
c2VsZWN0IGNvdW50KCopIGZyb20gKHNlbGVjdCBkaXN0aW5jdCBiIGZyb20gdCB3aGVyZSBj
dGlkIDwgJygxMjgsMCknOjp0aWQpIGZvbzsKCmNyZWF0ZSBpbmRleCBibG9vbV9hX2lkeCBv
biB0IHVzaW5nIGJyaW4gKGEgaW50NF9ibG9vbV9vcHMobl9kaXN0aW5jdF9wZXJfcmFuZ2U9
NjAwMCkpOyAKCmFuYWx5emUgdDsKCmV4cGxhaW4gc2VsZWN0ICogZnJvbSB0IHdoZXJlIGEg
PSAxMDAwOwoKY3JlYXRlIGluZGV4IGJyaW5faWR4IG9uIHQgdXNpbmcgYnJpbiAoYSk7CmFu
YWx5emUgdDsKCmV4cGxhaW4gc2VsZWN0ICogZnJvbSB0IHdoZXJlIGEgPSAxMDAwOwoKZHJv
cCBpbmRleCBicmluX2lkeDsKCmV4cGxhaW4gc2VsZWN0ICogZnJvbSB0IHdoZXJlIGEgPSAx
MDAwOwo=
--------------D524CD37E77B36A2883A7617--





^ permalink  raw  reply  [nested|flat] 3+ messages in thread

* [PATCH] Rework grammar for REINDEX
@ 2022-07-21 14:48  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 3+ messages in thread

From: Alvaro Herrera @ 2022-07-21 14:48 UTC (permalink / raw)

---
 src/backend/parser/gram.y                  | 80 +++++++++++-----------
 src/test/regress/expected/create_index.out |  4 +-
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d649a1b8d1..85ab17ca5a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -560,7 +560,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <defelt>	generic_option_elem alter_generic_option_elem
 %type <list>	generic_option_list alter_generic_option_list
 
-%type <ival>	reindex_target_type reindex_target_multitable reindex_name_optional
+%type <ival>	reindex_target_type reindex_target_type_multi
 
 %type <node>	copy_generic_opt_arg copy_generic_opt_arg_list_item
 %type <defelt>	copy_generic_opt_elem
@@ -9085,7 +9085,9 @@ DropTransformStmt: DROP TRANSFORM opt_if_exists FOR Typename LANGUAGE name opt_d
  *
  *		QUERY:
  *
- *		REINDEX [ (options) ] type [CONCURRENTLY] <name>
+ *		REINDEX [ (options) ] {TABLE | INDEX} [CONCURRENTLY] <name>
+ *		REINDEX [ (options) ] SCHEMA [CONCURRENTLY] <name>
+ *		REINDEX [ (options) ] {SYSTEM | DATABASE} [<name>]
  *****************************************************************************/
 
 ReindexStmt:
@@ -9102,37 +9104,6 @@ ReindexStmt:
 											makeDefElem("concurrently", NULL, @3));
 					$$ = (Node *) n;
 				}
-			| REINDEX reindex_target_multitable opt_concurrently name
-				{
-					ReindexStmt *n = makeNode(ReindexStmt);
-
-					n->kind = $2;
-					n->name = $4;
-					n->relation = NULL;
-					n->params = NIL;
-					if ($3)
-						n->params = lappend(n->params,
-											makeDefElem("concurrently", NULL, @3));
-					$$ = (Node *) n;
-				}
-			| REINDEX reindex_name_optional
-				{
-					ReindexStmt *n = makeNode(ReindexStmt);
-					n->kind = $2;
-					n->name = NULL;
-					n->relation = NULL;
-					n->params = NIL;
-					$$ = (Node *)n;
-				}
-			| REINDEX '(' utility_option_list ')' reindex_name_optional
-				{
-					ReindexStmt *n = makeNode(ReindexStmt);
-					n->kind = $5;
-					n->name = NULL;
-					n->relation = NULL;
-					n->params = $3;
-					$$ = (Node *)n;
-				}
 			| REINDEX '(' utility_option_list ')' reindex_target_type opt_concurrently qualified_name
 				{
 					ReindexStmt *n = makeNode(ReindexStmt);
@@ -9146,11 +9117,25 @@ ReindexStmt:
 											makeDefElem("concurrently", NULL, @6));
 					$$ = (Node *) n;
 				}
-			| REINDEX '(' utility_option_list ')' reindex_target_multitable opt_concurrently name
+
+			| REINDEX SCHEMA opt_concurrently name
 				{
 					ReindexStmt *n = makeNode(ReindexStmt);
 
-					n->kind = $5;
+					n->kind = REINDEX_OBJECT_SCHEMA;
+					n->name = $4;
+					n->relation = NULL;
+					n->params = NIL;
+					if ($3)
+						n->params = lappend(n->params,
+											makeDefElem("concurrently", NULL, @3));
+					$$ = (Node *) n;
+				}
+			| REINDEX '(' utility_option_list ')' SCHEMA opt_concurrently name
+				{
+					ReindexStmt *n = makeNode(ReindexStmt);
+
+					n->kind = REINDEX_OBJECT_SCHEMA;
 					n->name = $7;
 					n->relation = NULL;
 					n->params = $3;
@@ -9159,18 +9144,31 @@ ReindexStmt:
 											makeDefElem("concurrently", NULL, @6));
 					$$ = (Node *) n;
 				}
+			| REINDEX reindex_target_type_multi OptSchemaName
+				{
+					ReindexStmt *n = makeNode(ReindexStmt);
+					n->kind = $2;
+					n->name = NULL;
+					n->relation = NULL;
+					n->params = NIL;
+					$$ = (Node *)n;
+				}
+			| REINDEX '(' utility_option_list ')' reindex_target_type_multi OptSchemaName
+				{
+					ReindexStmt *n = makeNode(ReindexStmt);
+					n->kind = $5;
+					n->name = $6;
+					n->relation = NULL;
+					n->params = $3;
+					$$ = (Node *)n;
+				}
 		;
 reindex_target_type:
 			INDEX					{ $$ = REINDEX_OBJECT_INDEX; }
 			| TABLE					{ $$ = REINDEX_OBJECT_TABLE; }
 		;
-reindex_target_multitable:
-			SCHEMA					{ $$ = REINDEX_OBJECT_SCHEMA; }
-			| SYSTEM_P				{ $$ = REINDEX_OBJECT_SYSTEM; }
-			| DATABASE				{ $$ = REINDEX_OBJECT_DATABASE; }
-		;
 /* For these options the name is optional */
-reindex_name_optional:
+reindex_target_type_multi:
 			SYSTEM_P				{ $$ = REINDEX_OBJECT_SYSTEM; }
 			| DATABASE				{ $$ = REINDEX_OBJECT_DATABASE; }
 		;
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index d55aec3a1d..d53af31753 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2521,7 +2521,9 @@ ERROR:  cannot reindex system catalogs concurrently
 REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
 ERROR:  cannot reindex system catalogs concurrently
 REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-ERROR:  cannot reindex system catalogs concurrently
+ERROR:  syntax error at or near "CONCURRENTLY"
+LINE 1: REINDEX SYSTEM CONCURRENTLY postgres;
+                       ^
 -- Warns about catalog relations
 REINDEX SCHEMA CONCURRENTLY pg_catalog;
 WARNING:  cannot reindex system catalogs concurrently, skipping all
-- 
2.30.2


--tgsznch6xx5nynio--





^ permalink  raw  reply  [nested|flat] 3+ messages in thread

* [PATCH v2] Rework grammar for REINDEX
@ 2022-07-21 14:48  Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 3+ messages in thread

From: Alvaro Herrera @ 2022-07-21 14:48 UTC (permalink / raw)

---
 doc/src/sgml/ref/reindex.sgml              |  3 +-
 src/backend/parser/gram.y                  | 87 ++++++++--------------
 src/test/regress/expected/create_index.out |  6 ++
 src/test/regress/sql/create_index.sql      |  2 +
 4 files changed, 41 insertions(+), 57 deletions(-)

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index fcbda88149..6750eb6e47 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,7 +22,8 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
-REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] DATABASE [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] SYSTEM [ <replaceable class="parameter">name</replaceable> ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b761a5b5d2..c6f74bf24e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -564,7 +564,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <defelt>	generic_option_elem alter_generic_option_elem
 %type <list>	generic_option_list alter_generic_option_list
 
-%type <ival>	reindex_target_type reindex_target_multitable reindex_name_optional
+%type <ival>	reindex_target_type
+%type <list>	opt_reindex_option_list
 
 %type <node>	copy_generic_opt_arg copy_generic_opt_arg_list_item
 %type <defelt>	copy_generic_opt_elem
@@ -9091,95 +9092,69 @@ DropTransformStmt: DROP TRANSFORM opt_if_exists FOR Typename LANGUAGE name opt_d
  *
  *		QUERY:
  *
- *		REINDEX [ (options) ] type [CONCURRENTLY] <name>
+ *		REINDEX [ (options) ] {TABLE | INDEX | SCHEMA} [CONCURRENTLY] <name>
+ *		REINDEX [ (options) ] DATABASE [CONCURRENTLY] [<name>]
+ *		REINDEX [ (options) ] SYSTEM [<name>]
  *****************************************************************************/
 
 ReindexStmt:
-			REINDEX reindex_target_type opt_concurrently qualified_name
+			REINDEX opt_reindex_option_list reindex_target_type opt_concurrently qualified_name
 				{
 					ReindexStmt *n = makeNode(ReindexStmt);
 
-					n->kind = $2;
-					n->relation = $4;
+					n->kind = $3;
+					n->relation = $5;
 					n->name = NULL;
-					n->params = NIL;
-					if ($3)
+					n->params = $2;
+					if ($4)
 						n->params = lappend(n->params,
-											makeDefElem("concurrently", NULL, @3));
+											makeDefElem("concurrently", NULL, @4));
 					$$ = (Node *) n;
 				}
-			| REINDEX reindex_target_multitable opt_concurrently name
+			| REINDEX opt_reindex_option_list SCHEMA opt_concurrently name
 				{
 					ReindexStmt *n = makeNode(ReindexStmt);
 
-					n->kind = $2;
-					n->name = $4;
+					n->kind = REINDEX_OBJECT_SCHEMA;
+					n->name = $5;
 					n->relation = NULL;
-					n->params = NIL;
-					if ($3)
+					n->params = $2;
+					if ($4)
 						n->params = lappend(n->params,
-											makeDefElem("concurrently", NULL, @3));
+											makeDefElem("concurrently", NULL, @4));
 					$$ = (Node *) n;
 				}
-			| REINDEX reindex_name_optional
+			| REINDEX opt_reindex_option_list DATABASE opt_concurrently opt_single_name
 				{
 					ReindexStmt *n = makeNode(ReindexStmt);
-					n->kind = $2;
+					n->kind = REINDEX_OBJECT_DATABASE;
 					n->name = NULL;
 					n->relation = NULL;
-					n->params = NIL;
+					n->params = $2;
 					$$ = (Node *)n;
 				}
-			| REINDEX '(' utility_option_list ')' reindex_name_optional
+			| REINDEX opt_reindex_option_list SYSTEM_P opt_single_name
 				{
 					ReindexStmt *n = makeNode(ReindexStmt);
-					n->kind = $5;
+					n->kind = REINDEX_OBJECT_SYSTEM;
 					n->name = NULL;
 					n->relation = NULL;
-					n->params = $3;
+					n->params = $2;
 					$$ = (Node *)n;
 				}
-			| REINDEX '(' utility_option_list ')' reindex_target_type opt_concurrently qualified_name
-				{
-					ReindexStmt *n = makeNode(ReindexStmt);
-
-					n->kind = $5;
-					n->relation = $7;
-					n->name = NULL;
-					n->params = $3;
-					if ($6)
-						n->params = lappend(n->params,
-											makeDefElem("concurrently", NULL, @6));
-					$$ = (Node *) n;
-				}
-			| REINDEX '(' utility_option_list ')' reindex_target_multitable opt_concurrently name
-				{
-					ReindexStmt *n = makeNode(ReindexStmt);
-
-					n->kind = $5;
-					n->name = $7;
-					n->relation = NULL;
-					n->params = $3;
-					if ($6)
-						n->params = lappend(n->params,
-											makeDefElem("concurrently", NULL, @6));
-					$$ = (Node *) n;
-				}
 		;
 reindex_target_type:
 			INDEX					{ $$ = REINDEX_OBJECT_INDEX; }
 			| TABLE					{ $$ = REINDEX_OBJECT_TABLE; }
 		;
-reindex_target_multitable:
-			SCHEMA					{ $$ = REINDEX_OBJECT_SCHEMA; }
-			| SYSTEM_P				{ $$ = REINDEX_OBJECT_SYSTEM; }
-			| DATABASE				{ $$ = REINDEX_OBJECT_DATABASE; }
-		;
-/* For these options the name is optional */
-reindex_name_optional:
-			SYSTEM_P				{ $$ = REINDEX_OBJECT_SYSTEM; }
-			| DATABASE				{ $$ = REINDEX_OBJECT_DATABASE; }
-		;
+opt_reindex_option_list:
+			'(' utility_option_list ')'
+				{
+					$$ = $2;
+				}
+			| /* EMPTY */
+				{   $$ = NULL; }
+			;
 
 /*****************************************************************************
  *
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index d55aec3a1d..a913a1846f 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2521,6 +2521,12 @@ ERROR:  cannot reindex system catalogs concurrently
 REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
 ERROR:  cannot reindex system catalogs concurrently
 REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
+ERROR:  syntax error at or near "CONCURRENTLY"
+LINE 1: REINDEX SYSTEM CONCURRENTLY postgres;
+                       ^
+REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
+ERROR:  cannot reindex system catalogs concurrently
+REINDEX (CONCURRENTLY) SYSTEM;  -- ditto
 ERROR:  cannot reindex system catalogs concurrently
 -- Warns about catalog relations
 REINDEX SCHEMA CONCURRENTLY pg_catalog;
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index d8fded3d93..4b75790e47 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1072,6 +1072,8 @@ REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
 REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
 REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
 REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
+REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
+REINDEX (CONCURRENTLY) SYSTEM;  -- ditto
 -- Warns about catalog relations
 REINDEX SCHEMA CONCURRENTLY pg_catalog;
 
-- 
2.30.2


--rqpjcyunjs7vhsrq--





^ permalink  raw  reply  [nested|flat] 3+ messages in thread


end of thread, other threads:[~2022-07-21 14:48 UTC | newest]

Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-11 11:57 [PATCH 8/8] Consider selectivity in choose_bitmap_and Tomas Vondra <[email protected]>
2022-07-21 14:48 [PATCH] Rework grammar for REINDEX Alvaro Herrera <[email protected]>
2022-07-21 14:48 [PATCH v2] Rework grammar for REINDEX Alvaro Herrera <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox