On 2022-Jul-21, Dean Rasheed wrote:
> I tend to agree with Matthias' earlier point about avoiding code
> duplication in the grammar. Without going off and refactoring other
> parts of the grammar not related to this patch, it's still a slightly
> smaller, simpler change, and less code duplication, to do this using a
> new opt_stats_name production in the grammar, as in the attached.
>
> I also noticed a comment in CreateStatistics() that needed updating.
>
> Barring any further comments, I'll push this shortly.
Thanks. I was looking at the recently modified REINDEX syntax and
noticed there another spot for taking an optional name. I ended up
reusing OptSchemaName for that, as in the attached patch. I think
adding production-specific additional productions is pointless and
probably bloats the grammar. So let me +1 your push of the patch you
posted, just to keep things moving forward, but in addition I propose to
later rename OptSchemaName to something more generic and use it in these
three places.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
>From 143467419c19aea6a79db46da461aae4d9afabac Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 21 Jul 2022 16:48:51 +0200
Subject: [PATCH] Rework grammar for REINDEX
---
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
