From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>, 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 01:25:43 |
Message-ID: | 20201204012543.GW24052@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
> > struct ReindexIndexCallbackState
> > {
> > - int options; /* options from statement */
> > + bool concurrently;
> > Oid locked_table_oid; /* tracks previously locked table */
> > };
>
> Here also, I think that we should just pass down the full
> ReindexParams set.
Ok.
Regarding the REINDEX patch, I think this comment is misleading:
| /*
| * If the relation has a secondary toast rel, reindex that too while we
| * still hold the lock on the main table.
| */
| if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
| {
| /*
| * Note that this should fail if the toast relation is missing, so
| * reset REINDEXOPT_MISSING_OK.
|+ *
|+ * Even if table was moved to new tablespace, normally toast cannot move.
| */
|+ Oid toasttablespaceOid = allowSystemTableMods ? tablespaceOid : InvalidOid;
| result |= reindex_relation(toast_relid, flags,
|- options & ~(REINDEXOPT_MISSING_OK));
|+ options & ~(REINDEXOPT_MISSING_OK),
|+ toasttablespaceOid);
| }
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 ?
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().
--
Justin
Attachment | Content-Type | Size |
---|---|---|
v32-0001-ExecReindex-and-ReindexParams.patch | text/x-diff | 16.7 KB |
v32-0002-Allow-REINDEX-to-change-tablespace.patch | text/x-diff | 28.7 KB |
v32-0003-Refactor-and-reuse-set_rel_tablespace.patch | text/x-diff | 5.9 KB |
v32-0004-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch | text/x-diff | 23.9 KB |
v32-0005-Implement-vacuum-full-cluster-INDEX_TABLESPACE-t.patch | text/x-diff | 18.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2020-12-04 02:09:05 | Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2 |
Previous Message | Fujii Masao | 2020-12-04 01:21:35 | Re: Add Information during standby recovery conflicts |