Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Surafel Temesgen <surafel3000(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: 2019-09-19 11:44:34
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Michael,

Thank you for your comments.

On 19.09.2019 7:43, Michael Paquier wrote:
> On Wed, Sep 18, 2019 at 03:46:20PM +0300, Alexey Kondratov wrote:
>> Currently in Postgres SET TABLESPACE always comes with [ NOWAIT ] option, so
>> I hope it worth adding this option here for convenience. Added in the new
>> version.
> It seems to me that it would be good to keep the patch as simple as
> possible for its first version, and split it into two if you would
> like to add this new option instead of bundling both together. This
> makes the review of one and the other more simple.

OK, it makes sense. I would also prefer first patch as simple as
possible, but adding this NOWAIT option required only a few dozens of
lines, so I just bundled everything together. Anyway, I will split
patches if we decide to keep [ SET TABLESPACE ... [NOWAIT] ] grammar.

> Anyway, regarding
> the grammar, is SET TABLESPACE really our best choice here? What
> about:
> - TABLESPACE = foo, in parenthesis only?
> - Only using TABLESPACE, without SET at the end of the query?
> SET is used in ALTER TABLE per the set of subqueries available there,
> but that's not the case of REINDEX.

I like SET TABLESPACE grammar, because it already exists and used both
in ALTER TABLE and ALTER INDEX. Thus, if we once add 'ALTER INDEX
index_name REINDEX SET TABLESPACE' (as was proposed earlier in the
thread), then it will be consistent with 'REINDEX index_name SET
TABLESPACE'. If we use just plain TABLESPACE, then it may be misleading
in the following cases:

- REINDEX TABLE table_name TABLESPACE tablespace_name
- REINDEX (TABLESPACE = tablespace_name) TABLE table_name

since it may mean 'Reindex all indexes of table_name, that stored in the
tablespace_name', doesn't it?

However, I have rather limited experience with Postgres, so I doesn't

> +-- check that all relations moved to new tablespace
> +SELECT relname FROM pg_class
> +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE
> spcname='regress_tblspace')
> +AND relname IN ('regress_tblspace_test_tbl_idx');
> + relname
> +-------------------------------
> + regress_tblspace_test_tbl_idx
> +(1 row)
> Just to check one relation you could use \d with the relation (index
> or table) name.

Yes, \d outputs tablespace name if it differs from pg_default, but it
shows other information in addition, which is not necessary here. Also
its output has more chances to be changed later, which may lead to the
failed tests. This query output is more or less stable and new relations
may be easily added to tests if we once add tablespace change to
CLUSTER/VACUUM FULL. I can change test to use \d, but not sure that it
would reduce test output length or will be helpful for a future tests

> - ereport(ERROR,
> - errmsg("cannot reindex temporary tables of other
> - sessions")))
> I would keep the order of this operation in order with
> CheckTableNotInUse().

Sure, I haven't noticed that reordered these operations, thanks.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Kuntal Ghosh 2019-09-19 11:50:15 Re: subscriptionCheck failures on nightjar
Previous Message Ashutosh Sharma 2019-09-19 11:41:52 Re: Zedstore - compressed in-core columnar storage