agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: Mon, 25 Jan 2021 23:11:38 +0300
Message-ID: <690fa051803c071d213b0d07b5aa9f55@postgrespro.ru> (raw)
In-Reply-To: <YA58Qdgb3a7/yrOH@paquier.xyz>
References: <03f88f70618ce73e75837b6125a143f7@postgrespro.ru>
<20210120183439.GA21339@alvherre.pgsql>
<YAjb2kYBKH07H5Xv@paquier.xyz>
<bac6739d47f053fe04fbd8cf05283137@postgrespro.ru>
<e8f209aebf33f02ea118e509a935647a@postgrespro.ru>
<20210121212651.GY8560@telsasoft.com>
<944df7cc452fe0c34cf7820da4588d05@postgrespro.ru>
<YA58Qdgb3a7/yrOH@paquier.xyz>
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
Attachments:
[text/x-diff] v5-0001-Extract-common-part-from-ATExecSetTableSpaceNoSto.patch (4.9K, ../690fa051803c071d213b0d07b5aa9f55@postgrespro.ru/2-v5-0001-Extract-common-part-from-ATExecSetTableSpaceNoSto.patch)
download | inline diff:
From 39880842d7af31dcbfcffe7219250b31102955d5 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@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
view thread (154+ messages) latest in thread
Message-ID: <690fa051803c071d213b0d07b5aa9f55@postgrespro.ru>
Permalink: ../690fa051803c071d213b0d07b5aa9f55@postgrespro.ru/
Also on: postgresql.org/message-id/690fa051803c071d213b0d07b5aa9f55@postgrespro.ru
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, michael@paquier.xyz, pryzby@telsasoft.com, alvherre@alvh.no-ip.org, peter.eisentraut@enterprisedb.com, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, robertmhaas@gmail.com, a.korotkov@postgrespro.ru, sawada.mshk@gmail.com, jltallon@adv-solutions.net
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
In-Reply-To: <690fa051803c071d213b0d07b5aa9f55@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox