On 2021-01-25 11:07, Michael Paquier wrote:
On Fri, Jan 22, 2021 at 05:07:02PM +0300, Alexey Kondratov wrote:
I have updated patches accordingly and also simplified tablespaceOid
checks
and assignment in the newly added SetRelTableSpace(). Result is
attached as
two separate patches for an ease of review, but no objections to merge
them
and apply at once if everything is fine.
extern void SetRelationHasSubclass(Oid relationId, bool
relhassubclass);
+extern bool SetRelTableSpace(Oid reloid, Oid tablespaceOid);
Seeing SetRelationHasSubclass(), wouldn't it be more consistent to use
SetRelationTableSpace() as routine name?
I think that we should document that the caller of this routine had
better do a CCI once done to make the tablespace chage visible.
Except for those two nits, the patch needs an indentation run and some
style tweaks but its logic looks fine. So I'll apply that first
piece.
I updated comment with CCI info, did pgindent run and renamed new
function to SetRelationTableSpace(). New patch is attached.
+INSERT INTO regress_tblspace_test_tbl (num1, num2, t)
+ SELECT round(random()*100), random(), repeat('text', 1000000)
+ FROM generate_series(1, 10) s(i);
Repeating 1M times a text value is too costly for such a test. And as
even for empty tables there is one page created for toast indexes,
there is no need for that?
Yes, TOAST relation is created anyway. I just wanted to put some data
into a TOAST index, so REINDEX did some meaningful work there, not only
a new relfilenode creation. However you are right and this query
increases tablespace tests execution for more for more than 2 times on
my machine. I think that it is not really required.
This patch is introducing three new checks for system catalogs:
- don't use tablespace for mapped relations.
- don't use tablespace for system relations, except if
allowSystemTableMods.
- don't move non-shared relation to global tablespace.
For the non-concurrent case, all three checks are in reindex_index().
For the concurrent case, the two first checks are in
ReindexMultipleTables() and the third one is in
ReindexRelationConcurrently(). That's rather tricky to follow because
CONCURRENTLY is not allowed on system relations. I am wondering if it
would be worth an extra comment effort, or if there is a way to
consolidate that better.
Yeah, all these checks we complicated from the beginning. I will try to
find a better place tomorrow or put more info into the comments at
least.
I am also going to check/fix the remaining points regarding 002
tomorrow.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
From 39880842d7af31dcbfcffe7219250b31102955d5 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.alek...@gmail.com>
Date: Wed, 20 Jan 2021 20:21:12 +0300
Subject: [PATCH v5 1/2] Extract common part from ATExecSetTableSpaceNoStorage
for a future usage
---
src/backend/commands/tablecmds.c | 95 +++++++++++++++++++-------------
src/include/commands/tablecmds.h | 2 +
2 files changed, 58 insertions(+), 39 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8687e9a97c..ec9c440e4e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13291,6 +13291,59 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
list_free(reltoastidxids);
}
+/*
+ * SetRelationTableSpace - modify relation tablespace in the pg_class entry.
+ *
+ * 'reloid' is an Oid of relation to be modified.
+ * 'tablespaceOid' is an Oid of new tablespace.
+ *
+ * Catalog modification is done only if tablespaceOid is different from
+ * the currently set. Returned bool value is indicating whether any changes
+ * were made or not. Note that caller is responsible for doing
+ * CommandCounterIncrement() to make tablespace changes visible.
+ */
+bool
+SetRelationTableSpace(Oid reloid, Oid tablespaceOid)
+{
+ Relation pg_class;
+ HeapTuple tuple;
+ Form_pg_class rd_rel;
+ bool changed = false;
+
+ /* Get a modifiable copy of the relation's pg_class row. */
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ /* MyDatabaseTableSpace is stored as InvalidOid. */
+ if (tablespaceOid == MyDatabaseTableSpace)
+ tablespaceOid = InvalidOid;
+
+ /* No work if no change in tablespace. */
+ if (tablespaceOid != rd_rel->reltablespace)
+ {
+ /* Update the pg_class row. */
+ rd_rel->reltablespace = tablespaceOid;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+ /* Record dependency on tablespace. */
+ changeDependencyOnTablespace(RelationRelationId,
+ reloid, rd_rel->reltablespace);
+
+ changed = true;
+ }
+
+ InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+
+ return changed;
+}
+
/*
* Special handling of ALTER TABLE SET TABLESPACE for relations with no
* storage that have an interest in preserving tablespace.
@@ -13301,10 +13354,6 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
static void
ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace)
{
- HeapTuple tuple;
- Oid oldTableSpace;
- Relation pg_class;
- Form_pg_class rd_rel;
Oid reloid = RelationGetRelid(rel);
/*
@@ -13319,41 +13368,9 @@ ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace)
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("only shared relations can be placed in pg_global tablespace")));
- /*
- * No work if no change in tablespace.
- */
- oldTableSpace = rel->rd_rel->reltablespace;
- if (newTableSpace == oldTableSpace ||
- (newTableSpace == MyDatabaseTableSpace && oldTableSpace == 0))
- {
- InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
- return;
- }
-
- /* Get a modifiable copy of the relation's pg_class row */
- pg_class = table_open(RelationRelationId, RowExclusiveLock);
-
- tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
- if (!HeapTupleIsValid(tuple))
- elog(ERROR, "cache lookup failed for relation %u", reloid);
- rd_rel = (Form_pg_class) GETSTRUCT(tuple);
-
- /* update the pg_class row */
- rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ? InvalidOid : newTableSpace;
- CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
-
- /* Record dependency on tablespace */
- changeDependencyOnTablespace(RelationRelationId,
- reloid, rd_rel->reltablespace);
-
- InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
-
- heap_freetuple(tuple);
-
- table_close(pg_class, RowExclusiveLock);
-
- /* Make sure the reltablespace change is visible */
- CommandCounterIncrement();
+ if (SetRelationTableSpace(reloid, newTableSpace))
+ /* Make sure the reltablespace change is visible */
+ CommandCounterIncrement();
}
/*
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 08c463d3c4..4ee7853027 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -61,6 +61,8 @@ extern void ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_
extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass);
+extern bool SetRelationTableSpace(Oid reloid, Oid tablespaceOid);
+
extern ObjectAddress renameatt(RenameStmt *stmt);
extern ObjectAddress RenameConstraint(RenameStmt *stmt);
base-commit: 881933f194221abcce07fb134ebe8685e5bb58dd
--
2.20.1
