|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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"
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))
- errmsg("permission denied: \"%s\" is a system catalog",
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
of REINDEX CONCURRENTLY?
> 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
> the table, rather than after clustering and re-indexing.
Yes, I think it would be much more user-friendly.
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
|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.|