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>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, 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-12-04 18:40:31 |
Message-ID: | 30bbac97f3ebbf1642e089583e4e45ba@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
refactor-ExecReindex.txt | text/x-diff | 1.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Joel Jacobson | 2020-12-04 18:44:37 | Re: [PATCH] Add support for leading/trailing bytea trim()ing |
Previous Message | Alvaro Herrera | 2020-12-04 18:29:51 | Re: POC: Better infrastructure for automated testing of concurrency issues |