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

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>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, 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-09-23 16:43:01
Message-ID: a9d64111ffe72f51069d50e90d5bf029@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2020-09-09 18:36, Justin Pryzby wrote:
> Rebased on a6642b3ae: Add support for partitioned tables and indexes in
> REINDEX
>
> So now this includes the new functionality and test for reindexing a
> partitioned table onto a new tablespace. That part could use some
> additional
> review.
>

I have finally had a look on your changes regarding partitioned tables.

+set_rel_tablespace(Oid indexid, char *tablespace)
+{
+ Oid tablespaceOid = tablespace ? get_tablespace_oid(tablespace, false)
:
+ InvalidOid;

You pass a tablespace name to set_rel_tablespace(), but it is already
parsed into the Oid before. So I do not see why we need this extra work
here instead of just passing Oid directly.

Also set_rel_tablespace() does not check for a no-op case, i.e. if
requested tablespace is the same as before.

+ /*
+ * Set the new tablespace for the relation. Do that only in the
+ * case where the reindex caller wishes to enforce a new tablespace.
+ */
+ if (set_tablespace &&
+ tablespaceOid != iRel->rd_rel->reltablespace)

Just noticed that this check is not completely correct as well, since it
does not check for MyDatabaseTableSpace (stored as InvalidOid) logic.

I put these small fixes directly into the attached 0003.

Also, I thought about your comment above set_rel_tablespace() and did a
bit 'extreme' refactoring, which is attached as a separated patch 0004.
The only one doubtful change IMO is reordering of RelationDropStorage()
operation inside reindex_index(). However, it only schedules unlinking
of physical storage at transaction commit, so this refactoring seems to
be safe.

If there will be no objections I would merge it with 0003.

On 2020-09-09 16:03, Alexey Kondratov wrote:
> On 2020-09-09 15:22, Michael Paquier wrote:
>>
>> By the way, skimming through the patch set, I was wondering if we
>> could do the refactoring of patch 0005 as a first step
>>
>
> Yes, I did it with intention to put as a first patch, but wanted to
> get some feedback. It's easier to refactor the last patch without
> rebasing others.
>
>>
>> until I
>> noticed this part:
>> +common_option_name:
>> NonReservedWord { $$ = $1; }
>> | analyze_keyword { $$ = "analyze"; }
>> This is not a good idea as you make ANALYZE an option available for
>> all the commands involved in the refactoring. A portion of that could
>> be considered though, like the use of common_option_arg.
>>
>
> From the grammar perspective ANY option is available for any command
> that uses parenthesized option list. All the checks and validations
> are performed at the corresponding command code.
> This analyze_keyword is actually doing only an ANALYZE word
> normalization if it's used as an option. Why it could be harmful?
>

Michael has not replied since then, but he was relatively positive about
0005 initially, so I put it as a first patch now.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

Attachment Content-Type Size
v28-0006-Implement-vacuum-full-cluster-INDEX_TABLESPACE-t.patch text/x-diff 19.0 KB
v28-0005-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch text/x-diff 23.5 KB
v28-0004-Refactor-and-reuse-set_rel_tablespace.patch text/x-diff 6.6 KB
v28-0003-Allow-REINDEX-to-change-tablespace.patch text/x-diff 36.8 KB
v28-0002-Change-REINDEX-CLUSTER-to-accept-an-option-list.patch text/x-diff 18.5 KB
v28-0001-Refactor-gram.y-in-order-to-add-a-common-parenth.patch text/x-diff 4.9 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Takashi Menjo 2020-09-23 17:37:56 Re: [PoC] Non-volatile WAL buffer
Previous Message Alvaro Herrera 2020-09-23 15:23:21 Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY