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: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(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: 2021-01-27 21:19:06
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2021-01-27 06:14, Michael Paquier wrote:
> On Wed, Jan 27, 2021 at 01:00:50AM +0300, Alexey Kondratov wrote:
>> In the new 0002 I moved ACL check to the upper level, i.e.
>> ExecReindex(),
>> and removed expensive text generation in test. Not touched yet some of
>> your
>> previously raised concerns. Also, you made SetRelationTableSpace() to
>> accept
>> Relation instead of Oid, so now we have to open/close indexes in the
>> ReindexPartitions(), I am not sure that I use proper locking there,
>> but it
>> works.
> Passing down Relation to the new routines makes the most sense to me
> because we force the callers to think about the level of locking
> that's required when doing any tablespace moves.
> + Relation iRel = index_open(partoid, ShareLock);
> +
> + if (CheckRelationTableSpaceMove(iRel,
> params->tablespaceOid))
> + SetRelationTableSpace(iRel,
> + params->tablespaceOid,
> + InvalidOid);
> Speaking of which, this breaks the locking assumptions of
> SetRelationTableSpace(). I feel that we should think harder about
> this part for partitioned indexes and tables because this looks rather
> unsafe in terms of locking assumptions with partition trees. If we
> cannot come up with a safe solution, I would be fine with disallowing
> TABLESPACE in this case, as a first step. Not all problems have to be
> solved at once, and even without this part the feature is still
> useful.

I have read more about lock levels and ShareLock should prevent any kind
of physical modification of indexes. We already hold ShareLock doing
find_all_inheritors(), which is higher than ShareUpdateExclusiveLock, so
using ShareLock seems to be safe here, but I will look on it closer.

> + /* It's not a shared catalog, so refuse to move it to shared
> tablespace */
> + if (params->tablespaceOid == GLOBALTABLESPACE_OID)
> + ereport(ERROR,
> + errmsg("cannot move non-shared relation to tablespace
> \"%s\"",
> + get_tablespace_name(params->tablespaceOid))));
> Why is that needed if CheckRelationTableSpaceMove() is used?

This is from ReindexRelationConcurrently() where we do not use
CheckRelationTableSpaceMove(). For me it makes sense to add only this
GLOBALTABLESPACE_OID check there, since before we already check for
system catalogs and after for temp relations, so adding
CheckRelationTableSpaceMove() will be a double-check.

> - indexRelation->rd_rel->reltablespace,
> + OidIsValid(tablespaceOid) ?
> + tablespaceOid :
> indexRelation->rd_rel->reltablespace,
> Let's remove this logic from index_concurrently_create_copy() and let
> the caller directly decide the tablespace to use, without a dependency
> on InvalidOid in the inner routine. A share update exclusive lock is
> already hold on the old index when creating the concurrent copy, so
> there won't be concurrent schema changes.


Also added tests for ACL checks, relfilenode changes. Added ACL recheck
for multi-transactional case. Added info about TOAST index reindexing.
Changed some comments.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

Attachment Content-Type Size
v8-0001-Allow-REINDEX-to-change-tablespace.patch text/x-diff 32.1 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Bossart, Nathan 2021-01-27 21:29:08 Re: archive status ".ready" files may be created too early
Previous Message Andrew Dunstan 2021-01-27 20:42:10 Re: Allow matching whole DN from a client certificate