On 2020-04-06 21:44, Justin Pryzby wrote:
On Mon, Apr 06, 2020 at 08:43:46PM +0300, Alexey Kondratov wrote:
+/* XXX: reusing reindex_option_list */
+ | CLUSTER opt_verbose '(' reindex_option_list ')'
qualified_name
cluster_index_specification
Could we actually simply reuse vac_analyze_option_list? From the first
sight
it does just the right thing, excepting the special handling of
spelling
ANALYZE/ANALYSE, but it does not seem to be a problem.
Hm, do you mean to let cluster.c reject the other options like
"analyze" ?
I'm not sure why that would be better than reusing reindex?
I think the suggestion will probably be to just copy+paste the reindex
option
list and rename it to cluster (possibly with the explanation that
they're
separate and independant and so their behavior shouldn't be tied
together).
I mean to literally use vac_analyze_option_list for reindex and cluster
as well. Please, check attached 0007. Now, vacuum, reindex and cluster
filter options list and reject everything that is not supported, so it
seems completely fine to just reuse vac_analyze_option_list, doesn't it?
ReindexRelationConcurrently is used for all cases, but it hits
different
code paths in the case of database, table and index. I have not
checked yet,
but are you sure it is safe removing these validations in the case of
REINDEX CONCURRENTLY?
You're right about the pg_global case, fixed. System catalogs can't be
reindexed CONCURRENTLY, so they're already caught by that check.
> XXX: for cluster/vacuum, it might be more friendly to check before
> clustering
> the table, rather than after clustering and re-indexing.
Yes, I think it would be much more user-friendly.
I realized it's not needed or useful to check indexes in advance of
clustering,
since 1) a mapped index will be on a mapped relation, which is already
checked;
2) a system index will be on a system relation. Right ?
Yes, it seems that you are right. I have tried to create user index on
system relation with allow_system_table_mods=1, but this new index
appeared to become system as well. That way, we do not have to check
indexes in advance.
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
From afa132674d2b49328a5e95ecd2ff30d838f7ec6c Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.alek...@gmail.com>
Date: Tue, 7 Apr 2020 13:42:42 +0300
Subject: [PATCH v18 7/7] Reuse vac_analyze_option_list for cluster and reindex
---
src/backend/parser/gram.y | 38 +++-----------------------------------
1 file changed, 3 insertions(+), 35 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3665ee8700..7821f07c40 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -512,10 +512,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <list> explain_option_list
%type <ival> reindex_target_type reindex_target_multitable
-%type <str> reindex_option_name
-%type <node> reindex_option_arg
-%type <list> reindex_option_list
-%type <defelt> reindex_option_elem
%type <node> copy_generic_opt_arg copy_generic_opt_arg_list_item
%type <defelt> copy_generic_opt_elem
@@ -8428,7 +8424,7 @@ ReindexStmt:
n->params = NIL;
$$ = (Node *)n;
}
- | REINDEX '(' reindex_option_list ')' reindex_target_type opt_concurrently qualified_name
+ | REINDEX '(' vac_analyze_option_list ')' reindex_target_type opt_concurrently qualified_name
{
ReindexStmt *n = makeNode(ReindexStmt);
n->kind = $5;
@@ -8438,7 +8434,7 @@ ReindexStmt:
n->params = $3;
$$ = (Node *)n;
}
- | REINDEX '(' reindex_option_list ')' reindex_target_multitable opt_concurrently name
+ | REINDEX '(' vac_analyze_option_list ')' reindex_target_multitable opt_concurrently name
{
ReindexStmt *n = makeNode(ReindexStmt);
n->kind = $5;
@@ -8458,33 +8454,6 @@ reindex_target_multitable:
| SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; }
| DATABASE { $$ = REINDEX_OBJECT_DATABASE; }
;
-reindex_option_list:
- reindex_option_elem
- {
- $$ = list_make1($1);
- }
- | reindex_option_list ',' reindex_option_elem
- {
- $$ = lappend($1, $3);
- }
- ;
-
-reindex_option_elem:
- reindex_option_name reindex_option_arg
- {
- $$ = makeDefElem($1, $2, @1);
- }
- ;
-
-reindex_option_name:
- NonReservedWord { $$ = $1; }
- ;
-
-reindex_option_arg:
- opt_boolean_or_string { $$ = (Node *) makeString($1); }
- | NumericOnly { $$ = (Node *) $1; }
- | /* EMPTY */ { $$ = NULL; }
- ;
/*****************************************************************************
*
@@ -10643,8 +10612,7 @@ ClusterStmt:
n->params = NIL;
$$ = (Node*)n;
}
-/* XXX: reusing reindex_option_list */
- | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name cluster_index_specification
+ | CLUSTER opt_verbose '(' vac_analyze_option_list ')' qualified_name cluster_index_specification
{
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = $6;
--
2.19.1
