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>, 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-04-06 17:43:46
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2020-04-03 21:27, Justin Pryzby wrote:
> On Wed, Apr 01, 2020 at 08:08:36AM -0500, Justin Pryzby wrote:
>> Or maybe you'd want me to squish my changes into yours and resend
>> after any
>> review comments ?
> I didn't hear any feedback, so I've now squished all "parenthesized"
> and "fix"
> commits.

Thanks for the input, but I am afraid that the patch set became a bit
messy now. I have eyeballed it and found some inconsistencies.

const char *name; /* name of database to reindex */
- int options; /* Reindex options flags */
+ List *rawoptions; /* Raw options */
+ int options; /* Parsed options */
bool concurrent; /* reindex concurrently? */

You introduced rawoptions in the 0002, but then removed it in 0003. So
is it required or not? Probably this is a rebase artefact.

+/* XXX: reusing reindex_option_list */
+ | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name

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.

> 0004 reduces duplicative error handling, as a separate commit so
> Alexey can review it and/or integrate it.

@@ -2974,27 +2947,6 @@ ReindexRelationConcurrently(Oid relationOid, Oid
tablespaceOid, int options)
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
- /*
- * We don't support moving system relations into different
- * unless allow_system_table_mods=1.
- */
- if (OidIsValid(tablespaceOid) &&
- !allowSystemTableMods && IsSystemRelation(heapRelation))
- ereport(ERROR,
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(heapRelation))));

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

> The last two commits save a few
> dozen lines of code, but not sure they're desirable.

Sincerely, I do not think that passing raw strings down to the guts is a
good idea. Yes, it saves us a few checks here and there now, but it may
reduce a further reusability of these internal routines in the future.

> 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.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message David Steele 2020-04-06 18:12:54 Re: Allow cluster owner to bypass authentication
Previous Message Fujii Masao 2020-04-06 17:43:02 Re: Don't try fetching future segment of a TLI.