agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: pgsql-hackers@lists.postgresql.org, Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: Mon, 06 Apr 2020 20:43:46 +0300
Message-ID: <a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru> (raw)
In-Reply-To: <20200403182712.GR14618@telsasoft.com>
References: <39a055a7a8cd19552f35f4cb98a8cd80@postgrespro.ru>
<20200326180112.GH17431@telsasoft.com>
<20200327040106.GC20103@telsasoft.com>
<20200328001112.GX20103@telsasoft.com>
<f85b48beddc77c953f444cef06e1c16f@postgrespro.ru>
<20200330183439.GQ20103@telsasoft.com>
<624921b1ebca9897173d85c424c00646@postgrespro.ru>
<20200401060334.GB142683@paquier.xyz>
<20200401115718.GQ14618@telsasoft.com>
<20200401130836.GT14618@telsasoft.com>
<20200403182712.GR14618@telsasoft.com>
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
cluster_index_specification
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
tablespaces,
- * unless allow_system_table_mods=1.
- */
- if (OidIsValid(tablespaceOid) &&
- !allowSystemTableMods && IsSystemRelation(heapRelation))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 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
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
> clustering
> the table, rather than after clustering and re-indexing.
>
Yes, I think it would be much more user-friendly.
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
view thread (154+ messages) latest in thread
Message-ID: <a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru>
Permalink: ../a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru/
Also on: postgresql.org/message-id/a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, pryzby@telsasoft.com, michael@paquier.xyz, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, alvherre@2ndquadrant.com, robertmhaas@gmail.com, a.korotkov@postgrespro.ru, sawada.mshk@gmail.com, jltallon@adv-solutions.net
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
In-Reply-To: <a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox