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
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)
{
