On 2021-01-21 04:41, Michael Paquier wrote:
On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:
On 2021-Jan-20, Alexey Kondratov wrote:
Ugh, forgot to attach the patches. Here they are.
Yeah, looks reasonable.
+
+ if (changed)
+ /* Record dependency on tablespace */
+ changeDependencyOnTablespace(RelationRelationId,
+ reloid,
rd_rel->reltablespace);
Why have a separate "if (changed)" block here instead of merging with
the above?
Yep.
Sure, this is a refactoring artifact.
+ if (SetRelTablespace(reloid, newTableSpace))
+ /* Make sure the reltablespace change is visible */
+ CommandCounterIncrement();
At quick glance, I am wondering why you just don't do a CCI within
SetRelTablespace().
I did it that way for a better readability at first, since it looks more
natural when you do some change (SetRelTablespace) and then make them
visible with CCI. Second argument was that in the case of
reindex_index() we have to also call RelationAssumeNewRelfilenode() and
RelationDropStorage() before doing CCI and making the new tablespace
visible. And this part is critical, I guess.
+ 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 was referring to mapped relations mentioned in the previous
sentence. I have tried to rewrite this part and make it more specific in
my current version. Also added Justin's changes to the docs and comment.
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.
I added proper pg_tablespace_aclcheck()'s into the reindex_index() and
ReindexPartitions().
+ 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.
I agree that follow-up REINDEX should also reindex moved partitions,
since REINDEX (TABLESPACE ...) is still reindex at first. I will try to
put something about this part into the docs. Also I think that we cannot
be sure that nothing happened with already reindexed partitions between
two consequent REINDEX calls.
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.
Yes, sure, it makes sense.
+ *
+ * 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.
You are right, we do not move TOAST indexes now, since
IsSystemRelation() is true for TOAST indexes, so I thought that we
should not allow moving them without allow_system_table_mods=true. Now I
wonder why ALTER TABLE does that.
I am going to attach the new version of patch set today or tomorrow.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
