agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: pgsql-hackers@lists.postgresql.org, Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: Tue, 07 Apr 2020 15:40:18 +0300
Message-ID: <9b6a6cec134451550b74aa2ffd1446d0@postgrespro.ru> (raw)
In-Reply-To: <20200406184406.GF2228@telsasoft.com>
References: <20200327040106.GC20103@telsasoft.com>
<20200328001112.GX20103@telsasoft.com>
<f85b48beddc77c953f444cef06e1c16f@postgrespro.ru>
<20200330183439.GQ20103@telsasoft.com>
<624921b1ebca9897173d85c424c00646@postgrespro.ru>
<20200401060334.GB142683@paquier.xyz>
<20200401115718.GQ14618@telsasoft.com>
<20200401130836.GT14618@telsasoft.com>
<20200403182712.GR14618@telsasoft.com>
<a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru>
<20200406184406.GF2228@telsasoft.com>
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
Attachments:
[text/x-diff] v18-0007-Reuse-vac_analyze_option_list-for-cluster-and-re.patch (2.8K, ../9b6a6cec134451550b74aa2ffd1446d0@postgrespro.ru/2-v18-0007-Reuse-vac_analyze_option_list-for-cluster-and-re.patch)
download | inline diff:
From afa132674d2b49328a5e95ecd2ff30d838f7ec6c Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@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
view thread (154+ messages) latest in thread
Message-ID: <9b6a6cec134451550b74aa2ffd1446d0@postgrespro.ru>
Permalink: ../9b6a6cec134451550b74aa2ffd1446d0@postgrespro.ru/
Also on: postgresql.org/message-id/9b6a6cec134451550b74aa2ffd1446d0@postgrespro.ru
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, pryzby@telsasoft.com, michael@paquier.xyz, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, alvherre@2ndquadrant.com, robertmhaas@gmail.com, a.korotkov@postgrespro.ru, sawada.mshk@gmail.com, jltallon@adv-solutions.net
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
In-Reply-To: <9b6a6cec134451550b74aa2ffd1446d0@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox