From: | Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru> |
---|---|
To: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Jose Luis Tallon <jltallon(at)adv-solutions(dot)net> |
Subject: | Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly |
Date: | 2020-03-26 19:22:08 |
Message-ID: | 39c239a12c4dde0a3c78e2ab5d8eb55b@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2020-03-26 21:01, Justin Pryzby wrote:
>
>> @@ -262,7 +280,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
>> * and error messages should refer to the operation as VACUUM not
>> CLUSTER.
>> */
>> void
>> -cluster_rel(Oid tableOid, Oid indexOid, int options)
>> +cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, int
>> options)
>
> Add a comment here about the tablespaceOid parameter, like the other
> functions
> where it's added.
>
> The permission checking is kind of duplicitive, so I'd suggest to
> factor it
> out. Ideally we'd only have one place that checks for
> pg_global/system/mapped.
> It needs to check that it's not a system relation, or that
> system_table_mods
> are allowed, and in any case that if it's a mapped rel, that it's not
> being
> moved. I would pass a boolean indicating if the tablespace is being
> changed.
>
Yes, but I wanted to make sure first that all necessary validations are
there to do not miss something as I did last time. I do not like
repetitive code either, so I would like to introduce more common check
after reviewing the code as a whole.
>
> Another issue is this:
>> +VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable
>> class="parameter">new_tablespace</replaceable> ] [ <replaceable
>> class="parameter">table_and_columns</replaceable> [, ...] ]
> As you mentioned in your v1 patch, in the other cases, "tablespace
> [tablespace]" is added at the end of the command rather than in the
> middle. I
> wasn't able to make that work, maybe because "tablespace" isn't a fully
> reserved word (?). I didn't try with "SET TABLESPACE", although I
> understand
> it'd be better without "SET".
>
Initially I tried "SET TABLESPACE", but also failed to completely get
rid of shift/reduce conflicts. I will try to rewrite VACUUM's part again
with OptTableSpace. Maybe I will manage it this time.
I will take into account all your text edits as well.
Thanks
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2020-03-26 19:37:11 | Re: backup manifests |
Previous Message | Tom Lane | 2020-03-26 19:05:20 | Re: plan cache overhead on plpgsql expression |