public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/4] Allow CLUSTER and VACUUM FULL to change tablespace
4+ messages / 4 participants
[nested] [flat]
* [PATCH 3/4] Allow CLUSTER and VACUUM FULL to change tablespace
@ 2020-03-24 15:16 Alexey Kondratov <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Alexey Kondratov @ 2020-03-24 15:16 UTC (permalink / raw)
---
doc/src/sgml/ref/cluster.sgml | 19 +++++---
doc/src/sgml/ref/vacuum.sgml | 20 ++++++++
src/backend/commands/cluster.c | 57 +++++++++++++++++++++--
src/backend/commands/tablecmds.c | 5 +-
src/backend/commands/vacuum.c | 54 +++++++++++++++++++--
src/backend/parser/gram.y | 5 +-
src/backend/postmaster/autovacuum.c | 1 +
src/bin/psql/tab-complete.c | 9 +++-
src/include/commands/cluster.h | 1 +
src/include/commands/vacuum.h | 2 +
src/test/regress/input/tablespace.source | 23 ++++++++-
src/test/regress/output/tablespace.source | 37 ++++++++++++++-
12 files changed, 212 insertions(+), 21 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 5dd21a0189..6758ef97a6 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -28,6 +28,7 @@ CLUSTER [VERBOSE]
<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
+ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
</synopsis>
</refsynopsisdiv>
@@ -105,6 +106,15 @@ CLUSTER [VERBOSE]
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the table will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>VERBOSE</literal></term>
<listitem>
@@ -115,15 +125,10 @@ CLUSTER [VERBOSE]
</varlistentry>
<varlistentry>
- <term><replaceable class="parameter">boolean</replaceable></term>
+ <term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- Specifies whether the selected option should be turned on or off.
- You can write <literal>TRUE</literal>, <literal>ON</literal>, or
- <literal>1</literal> to enable the option, and <literal>FALSE</literal>,
- <literal>OFF</literal>, or <literal>0</literal> to disable it. The
- <replaceable class="parameter">boolean</replaceable> value can also
- be omitted, in which case <literal>TRUE</literal> is assumed.
+ The tablespace where the table will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 21ab57d880..5261a7c727 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -35,6 +35,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
INDEX_CLEANUP [ <replaceable class="parameter">boolean</replaceable> ]
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
+ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -255,6 +256,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the relation will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
@@ -299,6 +309,16 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">new_tablespace</replaceable></term>
+ <listitem>
+ <para>
+ The tablespace where the relation will be rebuilt.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 096a06f7b3..626321e3ac 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -33,11 +33,13 @@
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_tablespace.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
#include "commands/progress.h"
#include "commands/tablecmds.h"
+#include "commands/tablespace.h"
#include "commands/vacuum.h"
#include "miscadmin.h"
#include "optimizer/optimizer.h"
@@ -68,7 +70,8 @@ typedef struct
} RelToCluster;
-static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose);
+static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose,
+ Oid NewTableSpaceOid);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
@@ -105,6 +108,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
ListCell *lc;
ClusterParams params = {0};
bool verbose = false;
+ /* Name of tablespace to use for clustered relation. */
+ char *tablespaceName = NULL;
/* Parse option list */
foreach(lc, stmt->params)
@@ -113,6 +118,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
if (strcmp(opt->defname, "verbose") == 0)
verbose = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "tablespace") == 0)
+ tablespaceName = defGetString(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -123,6 +130,19 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
params.options = (verbose ? CLUOPT_VERBOSE : 0);
+ /* Select tablespace Oid to use. */
+ if (tablespaceName)
+ {
+ params.tablespaceOid = get_tablespace_oid(tablespaceName, false);
+
+ /* Can't move a non-shared relation into pg_global */
+ if (params.tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ tablespaceName)));
+ }
+
if (stmt->relation != NULL)
{
/* This is the single-relation case. */
@@ -272,6 +292,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
* If indexOid is InvalidOid, the table will be rewritten in physical order
* instead of index order. This is the new implementation of VACUUM FULL,
* and error messages should refer to the operation as VACUUM not CLUSTER.
+ *
+ * "tablespaceOid" is the tablespace where the relation will be rebuilt, or
+ * InvalidOid to use its current tablespace.
*/
void
cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
@@ -374,6 +397,23 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot cluster a shared catalog")));
+ if (OidIsValid(params->tablespaceOid) &&
+ !allowSystemTableMods && IsSystemRelation(OldHeap))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+ RelationGetRelationName(OldHeap))));
+
+ /*
+ * We cannot support moving mapped relations into different tablespaces.
+ * (In particular this eliminates all shared catalogs.)
+ */
+ if (OidIsValid(params->tablespaceOid) && RelationIsMapped(OldHeap))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot change tablespace of mapped relation \"%s\"",
+ RelationGetRelationName(OldHeap))));
+
/*
* Don't process temp tables of other backends ... their local buffer
* manager is not going to cope.
@@ -424,7 +464,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(OldHeap, indexOid, verbose);
+ rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid);
/* NB: rebuild_relation does table_close() on OldHeap */
@@ -573,7 +613,7 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* NB: this routine closes OldHeap at the right time; caller should not.
*/
static void
-rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
@@ -584,6 +624,10 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
TransactionId frozenXid;
MultiXactId cutoffMulti;
+ /* Use new tablespace if passed. */
+ if (OidIsValid(NewTablespaceOid))
+ tableSpace = NewTablespaceOid;
+
/* Mark the correct index as clustered */
if (OidIsValid(indexOid))
mark_index_clustered(OldHeap, indexOid, true);
@@ -1028,6 +1072,13 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
*/
Assert(!target_is_pg_class);
+ if (!allowSystemTableMods && IsSystemClass(r1, relform1) &&
+ relform1->reltablespace != relform2->reltablespace)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+ get_rel_name(r1))));
+
swaptemp = relform1->relfilenode;
relform1->relfilenode = relform2->relfilenode;
relform2->relfilenode = swaptemp;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8687e9a97c..75b04eb161 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13196,8 +13196,9 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
if (RelationIsMapped(rel))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move system relation \"%s\"",
- RelationGetRelationName(rel))));
+ errmsg("cannot change tablespace of mapped relation \"%s\"",
+ RelationGetRelationName(rel))));
+
/* Can't move a non-shared relation into pg_global */
if (newTableSpace == GLOBALTABLESPACE_OID)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 462f9a0f82..61565c3f14 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -31,13 +31,16 @@
#include "access/tableam.h"
#include "access/transam.h"
#include "access/xact.h"
+#include "catalog/catalog.h"
#include "catalog/namespace.h"
#include "catalog/pg_database.h"
#include "catalog/pg_inherits.h"
#include "catalog/pg_namespace.h"
+#include "catalog/pg_tablespace.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
#include "commands/vacuum.h"
+#include "commands/tablespace.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "pgstat.h"
@@ -106,6 +109,10 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
bool disable_page_skipping = false;
ListCell *lc;
+ /* Name and Oid of tablespace to use for relations after VACUUM FULL. */
+ char *tablespacename = NULL;
+ Oid tablespaceOid = InvalidOid;
+
/* Set default value */
params.index_cleanup = VACOPT_TERNARY_DEFAULT;
params.truncate = VACOPT_TERNARY_DEFAULT;
@@ -142,6 +149,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.index_cleanup = get_vacopt_ternary_value(opt);
else if (strcmp(opt->defname, "truncate") == 0)
params.truncate = get_vacopt_ternary_value(opt);
+ else if (strcmp(opt->defname, "tablespace") == 0)
+ tablespacename = defGetString(opt);
else if (strcmp(opt->defname, "parallel") == 0)
{
if (opt->arg == NULL)
@@ -202,6 +211,27 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("VACUUM FULL cannot be performed in parallel")));
+ /* Get tablespace Oid to use. */
+ if (tablespacename)
+ {
+ if ((params.options & VACOPT_FULL) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("incompatible TABLESPACE option"),
+ errdetail("TABLESPACE can only be used with VACUUM FULL.")));
+
+ tablespaceOid = get_tablespace_oid(tablespacename, false);
+
+ /* Can't move a non-shared relation into pg_global */
+ if (tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ tablespacename)));
+
+ }
+ params.tablespace_oid = tablespaceOid;
+
/*
* Make sure VACOPT_ANALYZE is specified if any column lists are present.
*/
@@ -1718,8 +1748,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
LOCKMODE lmode;
Relation onerel;
LockRelId onerelid;
- Oid toast_relid;
- Oid save_userid;
+ Oid toast_relid,
+ save_userid;
int save_sec_context;
int save_nestlevel;
@@ -1857,6 +1887,22 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
return true;
}
+ /*
+ * We don't support moving system relations into different tablespaces
+ * unless allow_system_table_mods=1.
+ */
+ if ((params->options & VACOPT_FULL) != 0 &&
+ OidIsValid(params->tablespace_oid) &&
+ IsSystemRelation(onerel) && !allowSystemTableMods)
+ {
+ params->tablespace_oid = InvalidOid;
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("skipping tablespace change of \"%s\"",
+ RelationGetRelationName(onerel)),
+ errdetail("Cannot move system relation, only VACUUM is performed.")));
+ }
+
/*
* Get a session-level lock too. This will protect our access to the
* relation across multiple transactions, so that we can vacuum the
@@ -1916,7 +1962,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
*/
if (params->options & VACOPT_FULL)
{
- ClusterParams cluster_params = {0};
+ ClusterParams cluster_params = {
+ .tablespaceOid = params->tablespace_oid,
+ };
/* close relation before vacuuming, but hold lock until commit */
relation_close(onerel, NoLock);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 31c95443a5..24755a38d2 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10493,8 +10493,9 @@ cluster_index_specification:
/*****************************************************************************
*
* QUERY:
- * VACUUM
- * ANALYZE
+ * VACUUM [FULL] [FREEZE] [VERBOSE] [ANALYZE] [ <table_and_columns> [, ...] ]
+ * VACUUM [(options)] [ <table_and_columns> [, ...] ]
+ * ANALYZE [VERBOSE] [ <table_and_columns> [, ...] ]
*
*****************************************************************************/
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 47e60ca561..a5d98c8c95 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2932,6 +2932,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age;
tab->at_params.is_wraparound = wraparound;
tab->at_params.log_min_duration = log_min_duration;
+ tab->at_params.tablespace_oid = InvalidOid;
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
tab->at_relname = NULL;
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index ac4b40561b..64a435865e 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2321,7 +2321,9 @@ psql_completion(const char *text, int start, int end)
* one word, so the above test is correct.
*/
if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
- COMPLETE_WITH("VERBOSE");
+ COMPLETE_WITH("TABLESPACE", "VERBOSE");
+ else if (TailMatches("TABLESPACE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
}
/* COMMENT */
@@ -3870,9 +3872,12 @@ psql_completion(const char *text, int start, int end)
if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
COMPLETE_WITH("FULL", "FREEZE", "ANALYZE", "VERBOSE",
"DISABLE_PAGE_SKIPPING", "SKIP_LOCKED",
- "INDEX_CLEANUP", "TRUNCATE", "PARALLEL");
+ "INDEX_CLEANUP", "TRUNCATE", "PARALLEL",
+ "TABLESPACE");
else if (TailMatches("FULL|FREEZE|ANALYZE|VERBOSE|DISABLE_PAGE_SKIPPING|SKIP_LOCKED|INDEX_CLEANUP|TRUNCATE"))
COMPLETE_WITH("ON", "OFF");
+ else if (TailMatches("TABLESPACE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
}
else if (HeadMatches("VACUUM") && TailMatches("("))
/* "VACUUM (" should be caught above, so assume we want columns */
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index a941f2accd..868dea5ecc 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -27,6 +27,7 @@
typedef struct ClusterParams
{
bits32 options; /* bitmask of CLUOPT_* */
+ Oid tablespaceOid; /* tablespace to rebuild relation, or InvalidOid */
} ClusterParams;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 191cbbd004..0934487d4b 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,6 +227,8 @@ typedef struct VacuumParams
* disabled.
*/
int nworkers;
+ Oid tablespace_oid; /* tablespace to use for relations
+ * rebuilt by VACUUM FULL */
} VacuumParams;
/* GUC parameters */
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 6e94ffe617..2244c1d51d 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -17,7 +17,7 @@ ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- f
ALTER TABLESPACE regress_tblspace RESET (random_page_cost = 2.0); -- fail
ALTER TABLESPACE regress_tblspace RESET (random_page_cost, effective_io_concurrency); -- ok
--- create table to test REINDEX with TABLESPACE change
+-- create table to test REINDEX, CLUSTER and VACUUM FULL with TABLESPACE change
CREATE TABLE regress_tblspace_test_tbl (num1 bigint, num2 double precision, num3 double precision);
INSERT INTO regress_tblspace_test_tbl (num1, num2, num3)
SELECT round(random()*100), random(), random()*42
@@ -47,11 +47,32 @@ REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_am; -- fail
REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx; -- fail
REINDEX (TABLESPACE regress_tblspace) TABLE pg_am; -- fail
+-- check that all indexes moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+
+-- check CLUSTER with TABLESPACE change
+CLUSTER (TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+CLUSTER (TABLESPACE regress_tblspace) pg_authid USING pg_authid_rolname_index; -- fail
+CLUSTER (TABLESPACE pg_global) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- fail
+
-- check that all relations moved to new tablespace
SELECT relname FROM pg_class
WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
ORDER BY relname;
+-- check VACUUM with TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+VACUUM (FULL, TABLESPACE pg_default) pg_authid; -- skip with warning
+VACUUM (ANALYSE, TABLESPACE pg_default); -- fail
+VACUUM (FULL, TABLESPACE pg_global) regress_tblspace_test_tbl; -- fail
+
+-- check that all tables moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+
-- move indexes back to pg_default tablespace
REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; -- ok
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 0927d1c7ad..889933a9c8 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -20,7 +20,7 @@ ERROR: unrecognized parameter "some_nonexistent_parameter"
ALTER TABLESPACE regress_tblspace RESET (random_page_cost = 2.0); -- fail
ERROR: RESET must not include values for parameters
ALTER TABLESPACE regress_tblspace RESET (random_page_cost, effective_io_concurrency); -- ok
--- create table to test REINDEX with TABLESPACE change
+-- create table to test REINDEX, CLUSTER and VACUUM FULL with TABLESPACE change
CREATE TABLE regress_tblspace_test_tbl (num1 bigint, num2 double precision, num3 double precision);
INSERT INTO regress_tblspace_test_tbl (num1, num2, num3)
SELECT round(random()*100), random(), random()*42
@@ -61,9 +61,44 @@ REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx; -- fail
ERROR: cannot move non-shared relation to tablespace "pg_global"
REINDEX (TABLESPACE regress_tblspace) TABLE pg_am; -- fail
ERROR: permission denied: "pg_am_name_index" is a system catalog
+-- check that all indexes moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+ relname
+-------------------------------
+ regress_tblspace_test_tbl_idx
+(1 row)
+
+-- check CLUSTER with TABLESPACE change
+CLUSTER (TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+CLUSTER (TABLESPACE regress_tblspace) pg_authid USING pg_authid_rolname_index; -- fail
+ERROR: cannot cluster a shared catalog
+CLUSTER (TABLESPACE pg_global) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- fail
+ERROR: cannot move non-shared relation to tablespace "pg_global"
-- check that all relations moved to new tablespace
SELECT relname FROM pg_class
WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+ relname
+-------------------------------
+ regress_tblspace_test_tbl
+ regress_tblspace_test_tbl_idx
+(2 rows)
+
+-- check VACUUM with TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+VACUUM (FULL, TABLESPACE pg_default) pg_authid; -- skip with warning
+WARNING: skipping tablespace change of "pg_authid"
+DETAIL: Cannot move system relation, only VACUUM is performed.
+VACUUM (ANALYSE, TABLESPACE pg_default); -- fail
+ERROR: incompatible TABLESPACE option
+DETAIL: TABLESPACE can only be used with VACUUM FULL.
+VACUUM (FULL, TABLESPACE pg_global) regress_tblspace_test_tbl; -- fail
+ERROR: cannot move non-shared relation to tablespace "pg_global"
+-- check that all tables moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
ORDER BY relname;
relname
-------------------------------
--
2.17.0
--tsOsTdHNUZQcU9Ye
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0004-Implement-vacuum-full-cluster-INDEX_TABLESPACE-table.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* RE: Synchronizing slots from primary to standby
@ 2023-12-21 13:07 Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2023-12-21 13:07 UTC (permalink / raw)
To: 'shveta malik' <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Amit Kapila <[email protected]>; +Cc: Peter Smith <[email protected]>; Drouvot, Bertrand <[email protected]>; Nisha Moond <[email protected]>; Bharath Rupireddy <[email protected]>; Peter Eisentraut <[email protected]>; Bruce Momjian <[email protected]>; Ashutosh Sharma <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Ajin Cherian <[email protected]>; Alvaro Herrera <[email protected]>
Dear Shveta,
Thanks for updating the patch! Here is my comments for v52-0002.
~~~~~
system-views.sgml
01.
```
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>sync_state</structfield> <type>char</type>
+ </para>
+ <para>
+ Defines slot synchronization state. This is meaningful on the physical
+ standby which has configured <xref linkend="guc-enable-syncslot"/> = true.
+ Possible values are:
+ <itemizedlist>
+ <listitem>
+ <para><literal>n</literal> = none for user created slots,
...
```
Hmm. I'm not sure why we must show a single character to a user. I'm OK for
pg_subscription.srsubstate because it is a "catalog" - the actual value would be
recorded in the heap. But pg_replication_slot is just a view so that we can replace
internal representations to other strings. E.g., pg_replication_slots.wal_status.
How about using {none, initialized, ready} or something?
~~~~~
postmaster.c
02. bgworker_should_start_now
```
+ if (start_time == BgWorkerStart_ConsistentState_HotStandby &&
+ pmState != PM_RUN)
+ return true;
```
I'm not sure the second condition is really needed. The line will be executed when
pmState is PM_HOT_STANDBY. Is there a possibility that pmState is changed around here?
~~~~~
libpqwalreceiver.c
03. PQWalReceiverFunctions
```
+ .walrcv_get_dbname_from_conninfo = libpqrcv_get_dbname_from_conninfo,
```
Just to confirm - is there a rule for ordering?
~~~~~
slotsync.c
04. SlotSyncWorkerCtx
```
typedef struct SlotSyncWorkerCtx
{
pid_t pid;
slock_t mutex;
} SlotSyncWorkerCtx;
SlotSyncWorkerCtx *SlotSyncWorker = NULL;
```
Per other files like launcher.c, should we use a name like "SlotSyncWorkerCtxStruct"?
05. SlotSyncWorkerRegister()
Your coding will work well, but there is another approach which validates
slotsync parameters here. In this case, the postmaster should exit ASAP. This can
notify that there are some wrong settings to users earlier. Thought?
06. wait_for_primary_slot_catchup
```
+ CHECK_FOR_INTERRUPTS();
+
+ /* Handle any termination request if any */
+ ProcessSlotSyncInterrupts(wrconn);
```
ProcessSlotSyncInterrupts() also has CHECK_FOR_INTERRUPTS(), so no need to call.
07. wait_for_primary_slot_catchup
```
+ /*
+ * XXX: Is waiting for 2 seconds before retrying enough or more or
+ * less?
+ */
+ rc = WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
+ 2000L,
+ WAIT_EVENT_REPL_SLOTSYNC_PRIMARY_CATCHUP);
+
+ ResetLatch(MyLatch);
+
+ /* Emergency bailout if postmaster has died */
+ if (rc & WL_POSTMASTER_DEATH)
+ proc_exit(1);
```
Is there any reasons not to use WL_EXIT_ON_PM_DEATH event? If not, you can use.
08. synchronize_slots
```
+ SpinLockAcquire(&WalRcv->mutex);
+ if (!WalRcv ||
+ (WalRcv->slotname[0] == '\0') ||
+ XLogRecPtrIsInvalid(WalRcv->latestWalEnd))
+ {
...
```
Assuming that WalRcv is still NULL. In this case, does the first SpinLockAcquire()
lead a segmentation fault?
09. synchronize_slots
```
+ elog(DEBUG2, "slot sync worker's query:%s \n", s.data);
```
The query is not dynamical one, so I think no need to print even if the debug
mode.
10. synchronize_one_slot
IIUC, this function can synchronize slots even if the used plugin on primary is
not installed on the secondary server. If the slot is created by the slotsync
worker, users will recognize it after the server is promoted and the decode is
starting. I felt it is not good specification. Can we detect in the validation
phase?
~~~~~
not the source code
11.
I tested the typical case - promoting a publisher from a below diagram.
A physical replication slot "physical" was specified as standby_slot_names.
```
node A (primary) --> node B (secondary)
|
|
node C (subscriber)
```
And after the promoting, below lines were periodically output on logfiles for
node B and C.
```
WARNING: replication slot "physical" specified in parameter "standby_slot_names" does not exist, ignoring
```
Do you have idea to suppress the warning? IIUC it is a normal behavior of the
walsender so that we cannot avoid the periodical outputs.
The steps of the test was as follows:
1. stop the node A via pg_ctl stop
2. promota the node B via pg_ctl promote
3. change the connection string of the subscription via ALTER SUBSCRIPTION ... CONNECTION ...
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Synchronizing slots from primary to standby
@ 2023-12-22 10:48 shveta malik <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: shveta malik @ 2023-12-22 10:48 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Zhijie Hou (Fujitsu) <[email protected]>; Amit Kapila <[email protected]>; Peter Smith <[email protected]>; Drouvot, Bertrand <[email protected]>; Nisha Moond <[email protected]>; Bharath Rupireddy <[email protected]>; Peter Eisentraut <[email protected]>; Bruce Momjian <[email protected]>; Ashutosh Sharma <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Ajin Cherian <[email protected]>; Alvaro Herrera <[email protected]>; shveta malik <[email protected]>
On Thu, Dec 21, 2023 at 6:37 PM Hayato Kuroda (Fujitsu)
<[email protected]> wrote:
>
> Dear Shveta,
>
> Thanks for updating the patch! Here is my comments for v52-0002.
Thanks for the feedback Kuroda-san. I have addressed these in v53.
> ~~~~~
> system-views.sgml
>
> 01.
>
> ```
> +
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>sync_state</structfield> <type>char</type>
> + </para>
> + <para>
> + Defines slot synchronization state. This is meaningful on the physical
> + standby which has configured <xref linkend="guc-enable-syncslot"/> = true.
> + Possible values are:
> + <itemizedlist>
> + <listitem>
> + <para><literal>n</literal> = none for user created slots,
> ...
> ```
>
> Hmm. I'm not sure why we must show a single character to a user. I'm OK for
> pg_subscription.srsubstate because it is a "catalog" - the actual value would be
> recorded in the heap. But pg_replication_slot is just a view so that we can replace
> internal representations to other strings. E.g., pg_replication_slots.wal_status.
> How about using {none, initialized, ready} or something?
Done.
> ~~~~~
> postmaster.c
>
> 02. bgworker_should_start_now
>
> ```
> + if (start_time == BgWorkerStart_ConsistentState_HotStandby &&
> + pmState != PM_RUN)
> + return true;
> ```
>
> I'm not sure the second condition is really needed. The line will be executed when
> pmState is PM_HOT_STANDBY. Is there a possibility that pmState is changed around here?
'case PM_RUN:' is a fall-through and thus we need to have this second
condition under 'case PM_HOT_STANDBY' for
BgWorkerStart_ConsistentState_HotStandby to avoid the worker getting
started on non-standby.
> ~~~~~
> libpqwalreceiver.c
>
> 03. PQWalReceiverFunctions
>
> ```
> + .walrcv_get_dbname_from_conninfo = libpqrcv_get_dbname_from_conninfo,
> ```
>
> Just to confirm - is there a rule for ordering?
No, I think. I am not aware of any.
> ~~~~~
> slotsync.c
>
> 04. SlotSyncWorkerCtx
>
> ```
> typedef struct SlotSyncWorkerCtx
> {
> pid_t pid;
> slock_t mutex;
> } SlotSyncWorkerCtx;
>
> SlotSyncWorkerCtx *SlotSyncWorker = NULL;
> ```
>
> Per other files like launcher.c, should we use a name like "SlotSyncWorkerCtxStruct"?
Modified.
> 05. SlotSyncWorkerRegister()
>
> Your coding will work well, but there is another approach which validates
> slotsync parameters here. In this case, the postmaster should exit ASAP. This can
> notify that there are some wrong settings to users earlier. Thought?
I think the postmaster should not exit. IMO, slot-sync worker being a
child process of postmaster, should not control start or exit of
postmaster. The worker should only exit itself if slot-sync GUCs are
not set. Have you seen any other case where postmaster exits if any of
its bgworker processes has invalid GUCs?
> 06. wait_for_primary_slot_catchup
>
> ```
> + CHECK_FOR_INTERRUPTS();
> +
> + /* Handle any termination request if any */
> + ProcessSlotSyncInterrupts(wrconn);
> ```
>
> ProcessSlotSyncInterrupts() also has CHECK_FOR_INTERRUPTS(), so no need to call.
yes, removed.
> 07. wait_for_primary_slot_catchup
>
> ```
> + /*
> + * XXX: Is waiting for 2 seconds before retrying enough or more or
> + * less?
> + */
> + rc = WaitLatch(MyLatch,
> + WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
> + 2000L,
> + WAIT_EVENT_REPL_SLOTSYNC_PRIMARY_CATCHUP);
> +
> + ResetLatch(MyLatch);
> +
> + /* Emergency bailout if postmaster has died */
> + if (rc & WL_POSTMASTER_DEATH)
> + proc_exit(1);
> ```
>
> Is there any reasons not to use WL_EXIT_ON_PM_DEATH event? If not, you can use.
I think we should use WL_EXIT_ON_PM_DEATH. Corrected now.
> 08. synchronize_slots
>
> ```
> + SpinLockAcquire(&WalRcv->mutex);
> + if (!WalRcv ||
> + (WalRcv->slotname[0] == '\0') ||
> + XLogRecPtrIsInvalid(WalRcv->latestWalEnd))
> + {
> ...
> ```
>
> Assuming that WalRcv is still NULL. In this case, does the first SpinLockAcquire()
> lead a segmentation fault?
It may. Thanks for pointing this out. Modified.
> 09. synchronize_slots
>
> ```
> + elog(DEBUG2, "slot sync worker's query:%s \n", s.data);
> ```
>
> The query is not dynamical one, so I think no need to print even if the debug
> mode.
Okay. Removed.
> 10. synchronize_one_slot
>
> IIUC, this function can synchronize slots even if the used plugin on primary is
> not installed on the secondary server. If the slot is created by the slotsync
> worker, users will recognize it after the server is promoted and the decode is
> starting. I felt it is not good specification. Can we detect in the validation
> phase?
Noted the concern. Let me review more on this. I will revert back.
> ~~~~~
> not the source code
>
> 11.
>
> I tested the typical case - promoting a publisher from a below diagram.
> A physical replication slot "physical" was specified as standby_slot_names.
>
> ```
> node A (primary) --> node B (secondary)
> |
> |
> node C (subscriber)
> ```
>
> And after the promoting, below lines were periodically output on logfiles for
> node B and C.
>
> ```
> WARNING: replication slot "physical" specified in parameter "standby_slot_names" does not exist, ignoring
> ```
It seems like you have set standby_slot_names on the standby, that is
why promoted standby is emitting this warning. It is not recommended
to set it on standby as it is the primary GUC. Having said that, I
understand that even on primary, we may get this repeated warning if
standby_slot_names is not set correctly. This WARNING is intentional,
as the user should know that this setting is wrong. So I am not sure
if we should suppress this. I would like to know what others think on
this.
> Do you have idea to suppress the warning? IIUC it is a normal behavior of the
> walsender so that we cannot avoid the periodical outputs.
>
> The steps of the test was as follows:
>
> 1. stop the node A via pg_ctl stop
> 2. promota the node B via pg_ctl promote
> 3. change the connection string of the subscription via ALTER SUBSCRIPTION ... CONNECTION ...
>
thanks
Shveta
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Synchronizing slots from primary to standby
@ 2023-12-26 08:49 Amit Kapila <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Amit Kapila @ 2023-12-26 08:49 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: shveta malik <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Peter Smith <[email protected]>; Drouvot, Bertrand <[email protected]>; Nisha Moond <[email protected]>; Bharath Rupireddy <[email protected]>; Peter Eisentraut <[email protected]>; Bruce Momjian <[email protected]>; Ashutosh Sharma <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Ajin Cherian <[email protected]>; Alvaro Herrera <[email protected]>
On Thu, Dec 21, 2023 at 6:37 PM Hayato Kuroda (Fujitsu)
<[email protected]> wrote:
>
> 10. synchronize_one_slot
>
> IIUC, this function can synchronize slots even if the used plugin on primary is
> not installed on the secondary server. If the slot is created by the slotsync
> worker, users will recognize it after the server is promoted and the decode is
> starting. I felt it is not good specification. Can we detect in the validation
> phase?
>
I think we should be able to detect it if we want but do we want to
add this restriction considering that users can always install the
required plugins after standby gets promoted? I think we can do either
way in this case but as we are not going to use these slots till the
standby node is promoted, it seems okay to validate the plugins after
promotion once users use the synced slots.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-12-26 08:49 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 15:16 [PATCH 3/4] Allow CLUSTER and VACUUM FULL to change tablespace Alexey Kondratov <[email protected]>
2023-12-21 13:07 RE: Synchronizing slots from primary to standby Hayato Kuroda (Fujitsu) <[email protected]>
2023-12-22 10:48 ` Re: Synchronizing slots from primary to standby shveta malik <[email protected]>
2023-12-26 08:49 ` Re: Synchronizing slots from primary to standby Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox