On Mon, Jan 18, 2021 at 02:37:57AM -0600, Justin Pryzby wrote: > Attached. I will re-review these myself tomorrow.
I have begun looking at 0001 and 0002...
+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Wouldn't it be better to do first the refactoring of 0002 and then
0001 so as REINDEX can use the new routine, instead of putting that
into a comment?
+ This specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" relations. If <literal>SCHEMA</literal>,
+ <literal>DATABASE</literal> or <literal>SYSTEM</literal> is specified,
then
+ all unsuitable relations will be skipped and a single
<literal>WARNING</literal>
+ will be generated.
What is an unsuitable relation? How can the end user know that?
This is missing ACL checks when moving the index into a new location,
so this requires some pg_tablespace_aclcheck() calls, and the other
patches share the same issue.
+ else if (partkind == RELKIND_PARTITIONED_TABLE)
+ {
+ Relation rel = table_open(partoid, ShareLock);
+ List *indexIds = RelationGetIndexList(rel);
+ ListCell *lc;
+
+ table_close(rel, NoLock);
+ foreach (lc, indexIds)
+ {
+ Oid indexid = lfirst_oid(lc);
+ (void) set_rel_tablespace(indexid, params->tablespaceOid);
+ }
+ }
This is really a good question. ReindexPartitions() would trigger one
transaction per leaf to work on. Changing the tablespace of the
partitioned table(s) before doing any work has the advantage to tell
any new partition to use the new tablespace. Now, I see a struggling
point here: what should we do if the processing fails in the middle of
the move, leaving a portion of the leaves in the previous tablespace?
On a follow-up reindex with the same command, should the command force
a reindex even on the partitions that have been moved? Or could there
be a point in skipping the partitions that are already on the new
tablespace and only process the ones on the previous tablespace? It
seems to me that the first scenario makes the most sense as currently
a REINDEX works on all the relations defined, though there could be
use cases for the second case. This should be documented, I think.
There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels of
partitioned tables, as well as a partitioned table with no leaves in
the cases dealt with.
+ *
+ * Even if a table's indexes were moved to a new tablespace, the index
+ * on its toast table is not normally moved.
*/
Still, REINDEX (TABLESPACE) TABLE should move all of them to be
consistent with ALTER TABLE SET TABLESPACE, but that's not the case
with this code, no? This requires proper test coverage, but there is
nothing of the kind in this patch.
--
Michael
signature.asc
Description: PGP signature
