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: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: pgsql-hackers@lists.postgresql.org, 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: Fri, 04 Dec 2020 21:40:31 +0300
Message-ID: <30bbac97f3ebbf1642e089583e4e45ba@postgrespro.ru> (raw)
In-Reply-To: <20201204012543.GW24052@telsasoft.com>
References: <20201031183611.GA22691@telsasoft.com>
<20201124153123.GO24052@telsasoft.com>
<X8TYkH2cJHzqFDGM@paquier.xyz>
<bfc3903c0e30ef4349ec5cfee625d8d5@postgrespro.ru>
<X8Wun1FBVgcqb6Fb@paquier.xyz>
<20201201054308.GC24052@telsasoft.com>
<X8XeRagOdYS3wCkT@paquier.xyz>
<X8g9L7ZNymScmziJ@paquier.xyz>
<20201203043008.GL24052@telsasoft.com>
<X8iP9cFW8xdqmQsr@paquier.xyz>
<20201204012543.GW24052@telsasoft.com>
On 2020-12-04 04:25, Justin Pryzby wrote:
> On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
>> > +typedef struct ReindexParams {
>> > + bool concurrently;
>> > + bool verbose;
>> > + bool missingok;
>> > +
>> > + int options; /* bitmask of lowlevel REINDEXOPT_* */
>> > +} ReindexParams;
>> > +
>>
>> By moving everything into indexcmds.c, keeping ReindexParams within it
>> makes sense to me. Now, there is no need for the three booleans
>> because options stores the same information, no?
>
> I liked the bools, but dropped them so the patch is smaller.
>
I had a look on 0001 and it looks mostly fine to me except some strange
mixture of tabs/spaces in the ExecReindex(). There is also a couple of
meaningful comments:
- options =
- (verbose ? REINDEXOPT_VERBOSE : 0) |
- (concurrently ? REINDEXOPT_CONCURRENTLY : 0);
+ if (verbose)
+ params.options |= REINDEXOPT_VERBOSE;
Why do we need this intermediate 'verbose' variable here? We only use it
once to set a bitmask. Maybe we can do it like this:
params.options |= defGetBoolean(opt) ?
REINDEXOPT_VERBOSE : 0;
See also attached txt file with diff (I wonder can I trick cfbot this
way, so it does not apply the diff).
+ int options; /* bitmask of lowlevel REINDEXOPT_* */
I would prefer if the comment says '/* bitmask of ReindexOption */' as
in the VacuumOptions, since citing the exact enum type make it easier to
navigate source code.
>
> Regarding the REINDEX patch, I think this comment is misleading:
>
> |+ * Even if table was moved to new tablespace,
> normally toast cannot move.
> | */
> |+ Oid toasttablespaceOid = allowSystemTableMods ?
> tablespaceOid : InvalidOid;
> | result |= reindex_relation(toast_relid, flags,
>
> I think it ought to say "Even if a table's indexes were moved to a new
> tablespace, its toast table's index is not normally moved"
> Right ?
>
Yes, I think so, we are dealing only with index tablespace changing
here. Thanks for noticing.
>
> Also, I don't know whether we should check for GLOBALTABLESPACE_OID
> after
> calling get_tablespace_oid(), or in the lowlevel routines. Note that
> reindex_relation is called during cluster/vacuum, and in the later
> patches, I
> moved the test from from cluster() and ExecVacuum() to
> rebuild_relation().
>
IIRC, I wanted to do GLOBALTABLESPACE_OID check as early as possible
(just after getting Oid), since it does not make sense to proceed
further if tablespace is set to that value. So initially there were a
lot of duplicative GLOBALTABLESPACE_OID checks, since there were a lot
of reindex entry-points (index, relation, concurrently, etc.). Now we
are going to have ExecReindex(), so there are much less entry-points and
in my opinion it is fine to keep this validation just after
get_tablespace_oid().
However, this is mostly a sanity check. I can hardly imagine a lot of
users trying to constantly move indexes to the global tablespace, so it
is also OK to put this check deeper into guts.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Attachments:
[text/x-diff] refactor-ExecReindex.txt (1.6K, ../30bbac97f3ebbf1642e089583e4e45ba@postgrespro.ru/2-refactor-ExecReindex.txt)
download | inline diff:
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a27f8f9d83..0b1884815c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2472,8 +2472,6 @@ void
ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel)
{
ReindexParams params = {0};
- bool verbose = false,
- concurrently = false;
ListCell *lc;
char *tablespace = NULL;
@@ -2483,9 +2481,11 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel)
DefElem *opt = (DefElem *) lfirst(lc);
if (strcmp(opt->defname, "verbose") == 0)
- verbose = defGetBoolean(opt);
+ params.options |= defGetBoolean(opt) ?
+ REINDEXOPT_VERBOSE : 0;
else if (strcmp(opt->defname, "concurrently") == 0)
- concurrently = defGetBoolean(opt);
+ params.options |= defGetBoolean(opt) ?
+ REINDEXOPT_CONCURRENTLY : 0;
else if (strcmp(opt->defname, "tablespace") == 0)
tablespace = defGetString(opt);
else
@@ -2496,18 +2496,12 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel)
parser_errposition(pstate, opt->location)));
}
- if (verbose)
- params.options |= REINDEXOPT_VERBOSE;
+ params.tablespaceOid = tablespace ?
+ get_tablespace_oid(tablespace, false) : InvalidOid;
- if (concurrently)
- {
- params.options |= REINDEXOPT_CONCURRENTLY;
+ if (params.options & REINDEXOPT_CONCURRENTLY)
PreventInTransactionBlock(isTopLevel,
"REINDEX CONCURRENTLY");
- }
-
- params.tablespaceOid = tablespace ?
- get_tablespace_oid(tablespace, false) : InvalidOid;
switch (stmt->kind)
{
view thread (154+ messages) latest in thread
Message-ID: <30bbac97f3ebbf1642e089583e4e45ba@postgrespro.ru>
Permalink: ../30bbac97f3ebbf1642e089583e4e45ba@postgrespro.ru/
Also on: postgresql.org/message-id/30bbac97f3ebbf1642e089583e4e45ba@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, alvherre@2ndquadrant.com, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, 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: <30bbac97f3ebbf1642e089583e4e45ba@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