On 2019-Apr-23, Alvaro Herrera wrote:
> I'm not sure yet that 100% of the patch is gone, but yes much of it
> would go away thankfully.
Of course, the part that fixes the bug that indexes move tablespace when
recreated by a rewriting ALTER TABLE is still necessary. Included in
the attached patch.
(I think it would be good to have the relation being complained about in
the error message, though that requires passing the name to
GetDefaultTablespace.)
> I do suggest we should raise an error if rule a3 hits and it mentions
> the database tablespace (I stupidly forgot this critical point in the
> previous email). I think astonishment is lesser that way.
As in the attached. When pg_default is the database tablespace, these
cases fail with the patch, as expected:
alvherre=# CREATE TABLE q (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE
pg_default;
psql: ERROR: cannot specify default tablespace for partitioned relations
alvherre=# CREATE TABLE q (a int PRIMARY KEY USING INDEX TABLESPACE pg_default)
PARTITION BY LIST (a);
psql: ERROR: cannot specify default tablespace for partitioned relations
alvherre=# SET default_tablespace TO 'pg_default';
alvherre=# CREATE TABLE q (a int PRIMARY KEY) PARTITION BY LIST (a) ;
psql: ERROR: cannot specify default tablespace for partitioned relations
alvherre=# CREATE TABLE q (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE
foo;
psql: ERROR: cannot specify default tablespace for partitioned relations
alvherre=# CREATE TABLE q (a int PRIMARY KEY USING INDEX TABLESPACE foo)
PARTITION BY LIST (a);
psql: ERROR: cannot specify default tablespace for partitioned relations
These cases work:
alvherre=# CREATE TABLE q (a int PRIMARY KEY USING INDEX TABLESPACE foo)
PARTITION BY LIST (a) TABLESPACE foo;
alvherre=# SET default_tablespace TO ''; -- the default
alvherre=# CREATE TABLE q (a int PRIMARY KEY) PARTITION BY LIST (a);
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index 6d7e11645d2..4f2587d74af 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -314,6 +314,7 @@ Boot_DeclareIndexStmt:
stmt->transformed = false;
stmt->concurrent = false;
stmt->if_not_exists = false;
+ stmt->reset_default_tblspc = false;
/* locks and races need not concern us in bootstrap mode */
relationId = RangeVarGetRelid(stmt->relation, NoLock,
@@ -363,6 +364,7 @@ Boot_DeclareUniqueIndexStmt:
stmt->transformed = false;
stmt->concurrent = false;
stmt->if_not_exists = false;
+ stmt->reset_default_tblspc = false;
/* locks and races need not concern us in bootstrap mode */
relationId = RangeVarGetRelid(stmt->relation, NoLock,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a1c91b5fb87..3599c0d8ce0 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -467,8 +467,21 @@ DefineIndex(Oid relationId,
LOCKTAG heaplocktag;
LOCKMODE lockmode;
Snapshot snapshot;
+ int save_nestlevel = -1;
int i;
+ /*
+ * Some callers need us to run with an empty default_tablespace; this is a
+ * necessary hack to be able to reproduce catalog state accurately when
+ * recreating indexes after table-rewriting ALTER TABLE.
+ */
+ if (stmt->reset_default_tblspc)
+ {
+ save_nestlevel = NewGUCNestLevel();
+ (void) set_config_option("default_tablespace", "",
+ PGC_USERSET, PGC_S_SESSION,
+ GUC_ACTION_SAVE, true, 0, false);
+ }
/*
* Start progress report. If we're building a partition, this was already
@@ -622,10 +635,15 @@ DefineIndex(Oid relationId,
if (stmt->tableSpace)
{
tablespaceId = get_tablespace_oid(stmt->tableSpace, false);
+ if (partitioned && tablespaceId == MyDatabaseTableSpace)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot specify default tablespace for partitioned relation")));
}
else
{
- tablespaceId = GetDefaultTablespace(rel->rd_rel->relpersistence);
+ tablespaceId = GetDefaultTablespace(rel->rd_rel->relpersistence,
+ partitioned);
/* note InvalidOid is OK in this case */
}
@@ -980,6 +998,13 @@ DefineIndex(Oid relationId,
ObjectAddressSet(address, RelationRelationId, indexRelationId);
+ /*
+ * Revert to original default_tablespace. Must do this before any return
+ * from this function, but after index_create, so this is a good time.
+ */
+ if (save_nestlevel >= 0)
+ AtEOXact_GUC(true, save_nestlevel);
+
if (!OidIsValid(indexRelationId))
{
table_close(rel, NoLock);
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 2aac63296bf..99bf3c29f27 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -284,7 +284,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
/* Concurrent refresh builds new data in temp tablespace, and does diff. */
if (concurrent)
{
- tableSpace = GetDefaultTablespace(RELPERSISTENCE_TEMP);
+ tableSpace = GetDefaultTablespace(RELPERSISTENCE_TEMP, false);
relpersistence = RELPERSISTENCE_TEMP;
}
else
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index aa7328ea400..66122afb077 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -567,6 +567,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Datum reloptions;
ListCell *listptr;
AttrNumber attnum;
+ bool partitioned;
static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
Oid ofTypeId;
ObjectAddress address;
@@ -595,7 +596,10 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
elog(ERROR, "unexpected relkind: %d", (int) relkind);
relkind = RELKIND_PARTITIONED_TABLE;
+ partitioned = true;
}
+ else
+ partitioned = false;
/*
* Look up the namespace in which we are supposed to create the relation,
@@ -664,31 +668,24 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
if (stmt->tablespacename)
{
tablespaceId = get_tablespace_oid(stmt->tablespacename, false);
+
+ if (partitioned && tablespaceId == MyDatabaseTableSpace)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot specify default tablespace for partitioned relations")));
}
else if (stmt->partbound)
{
- HeapTuple tup;
-
/*
* For partitions, when no other tablespace is specified, we default
* the tablespace to the parent partitioned table's.
*/
Assert(list_length(inheritOids) == 1);
- tup = SearchSysCache1(RELOID,
- DatumGetObjectId(linitial_oid(inheritOids)));
-
- tablespaceId = ((Form_pg_class) GETSTRUCT(tup))->reltablespace;
-
- if (!OidIsValid(tablespaceId))
- tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence);
-
- ReleaseSysCache(tup);
+ tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}
else
- {
- tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence);
- /* note InvalidOid is OK in this case */
- }
+ tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
+ partitioned);
/* Check permissions except when using database's default */
if (OidIsValid(tablespaceId) && tablespaceId != MyDatabaseTableSpace)
@@ -825,7 +822,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
{
accessMethod = stmt->accessMethod;
- if (relkind == RELKIND_PARTITIONED_TABLE)
+ if (partitioned)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("specifying a table access method is not supported on a partitioned table")));
@@ -998,7 +995,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* Process the partitioning specification (if any) and store the partition
* key information into the catalog.
*/
- if (stmt->partspec)
+ if (partitioned)
{
ParseState *pstate;
char strategy;
@@ -11276,6 +11273,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
if (!rewrite)
TryReuseIndex(oldId, stmt);
+ stmt->reset_default_tblspc = true;
/* keep the index's comment */
stmt->idxcomment = GetComment(oldId, RelationRelationId, 0);
@@ -11307,6 +11305,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
/* keep any comment on the index */
indstmt->idxcomment = GetComment(indoid,
RelationRelationId, 0);
+ indstmt->reset_default_tblspc = true;
cmd->subtype = AT_ReAddIndex;
tab->subcmds[AT_PASS_OLD_INDEX] =
@@ -11329,6 +11328,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
if (con->contype == CONSTR_FOREIGN &&
!rewrite && tab->rewrite == 0)
TryReuseForeignKey(oldId, con);
+ con->reset_default_tblspc = true;
cmd->subtype = AT_ReAddConstraint;
tab->subcmds[AT_PASS_OLD_CONSTR] =
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 3784ea4b4fa..8ec963f1cfb 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -1104,7 +1104,9 @@ check_default_tablespace(char **newval, void **extra, GucSource source)
* GetDefaultTablespace -- get the OID of the current default tablespace
*
* Temporary objects have different default tablespaces, hence the
- * relpersistence parameter must be specified.
+ * relpersistence parameter must be specified. Also, for partitioned tables,
+ * we disallow specifying the database default, so that needs to be specified
+ * too.
*
* May return InvalidOid to indicate "use the database's default tablespace".
*
@@ -1115,7 +1117,7 @@ check_default_tablespace(char **newval, void **extra, GucSource source)
* default_tablespace GUC variable.
*/
Oid
-GetDefaultTablespace(char relpersistence)
+GetDefaultTablespace(char relpersistence, bool partitioned)
{
Oid result;
@@ -1141,10 +1143,18 @@ GetDefaultTablespace(char relpersistence)
/*
* Allow explicit specification of database's default tablespace in
- * default_tablespace without triggering permissions checks.
+ * default_tablespace without triggering permissions checks. Don't
+ * allow specifying that when creating a partitioned table, however,
+ * since the result is confusing.
*/
if (result == MyDatabaseTableSpace)
+ {
+ if (partitioned)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot specify default tablespace for partitioned relations")));
result = InvalidOid;
+ }
return result;
}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 8f51315bee8..780d7ab00b5 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2919,6 +2919,7 @@ _copyConstraint(const Constraint *from)
COPY_NODE_FIELD(options);
COPY_STRING_FIELD(indexname);
COPY_STRING_FIELD(indexspace);
+ COPY_SCALAR_FIELD(reset_default_tblspc);
COPY_STRING_FIELD(access_method);
COPY_NODE_FIELD(where_clause);
COPY_NODE_FIELD(pktable);
@@ -3475,6 +3476,7 @@ _copyIndexStmt(const IndexStmt *from)
COPY_SCALAR_FIELD(transformed);
COPY_SCALAR_FIELD(concurrent);
COPY_SCALAR_FIELD(if_not_exists);
+ COPY_SCALAR_FIELD(reset_default_tblspc);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 68b51f3de71..4f2ebe5118e 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1347,6 +1347,7 @@ _equalIndexStmt(const IndexStmt *a, const IndexStmt *b)
COMPARE_SCALAR_FIELD(transformed);
COMPARE_SCALAR_FIELD(concurrent);
COMPARE_SCALAR_FIELD(if_not_exists);
+ COMPARE_SCALAR_FIELD(reset_default_tblspc);
return true;
}
@@ -2593,6 +2594,7 @@ _equalConstraint(const Constraint *a, const Constraint *b)
COMPARE_NODE_FIELD(options);
COMPARE_STRING_FIELD(indexname);
COMPARE_STRING_FIELD(indexspace);
+ COMPARE_SCALAR_FIELD(reset_default_tblspc);
COMPARE_STRING_FIELD(access_method);
COMPARE_NODE_FIELD(where_clause);
COMPARE_NODE_FIELD(pktable);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 82ca6826ab1..387e4b9b716 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2649,6 +2649,7 @@ _outIndexStmt(StringInfo str, const IndexStmt *node)
WRITE_BOOL_FIELD(transformed);
WRITE_BOOL_FIELD(concurrent);
WRITE_BOOL_FIELD(if_not_exists);
+ WRITE_BOOL_FIELD(reset_default_tblspc);
}
static void
@@ -3491,6 +3492,7 @@ _outConstraint(StringInfo str, const Constraint *node)
WRITE_NODE_FIELD(options);
WRITE_STRING_FIELD(indexname);
WRITE_STRING_FIELD(indexspace);
+ WRITE_BOOL_FIELD(reset_default_tblspc);
/* access_method and where_clause not currently used */
break;
@@ -3501,6 +3503,7 @@ _outConstraint(StringInfo str, const Constraint *node)
WRITE_NODE_FIELD(options);
WRITE_STRING_FIELD(indexname);
WRITE_STRING_FIELD(indexspace);
+ WRITE_BOOL_FIELD(reset_default_tblspc);
/* access_method and where_clause not currently used */
break;
@@ -3511,6 +3514,7 @@ _outConstraint(StringInfo str, const Constraint *node)
WRITE_NODE_FIELD(options);
WRITE_STRING_FIELD(indexname);
WRITE_STRING_FIELD(indexspace);
+ WRITE_BOOL_FIELD(reset_default_tblspc);
WRITE_STRING_FIELD(access_method);
WRITE_NODE_FIELD(where_clause);
break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b51f12dc232..3dc0e8a4fbc 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -7363,6 +7363,7 @@ IndexStmt: CREATE opt_unique INDEX opt_concurrently opt_index_name
n->initdeferred = false;
n->transformed = false;
n->if_not_exists = false;
+ n->reset_default_tblspc = false;
$$ = (Node *)n;
}
| CREATE opt_unique INDEX opt_concurrently IF_P NOT EXISTS index_name
@@ -7390,6 +7391,7 @@ IndexStmt: CREATE opt_unique INDEX opt_concurrently opt_index_name
n->initdeferred = false;
n->transformed = false;
n->if_not_exists = true;
+ n->reset_default_tblspc = false;
$$ = (Node *)n;
}
;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 23996904c47..4564c0ae815 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1406,6 +1406,7 @@ generateClonedIndexStmt(RangeVar *heapRel, Relation source_idx,
index->transformed = true; /* don't need transformIndexStmt */
index->concurrent = false;
index->if_not_exists = false;
+ index->reset_default_tblspc = false;
/*
* We don't try to preserve the name of the source index; instead, just
@@ -2001,6 +2002,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
index->transformed = false;
index->concurrent = false;
index->if_not_exists = false;
+ index->reset_default_tblspc = constraint->reset_default_tblspc;
/*
* If it's ALTER TABLE ADD CONSTRAINT USING INDEX, look up the index and
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 0c7a533e697..1e3bcb47b86 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1429,12 +1429,13 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
Oid tblspc;
tblspc = get_rel_tablespace(indexrelid);
- if (!OidIsValid(tblspc))
- tblspc = MyDatabaseTableSpace;
- if (isConstraint)
- appendStringInfoString(&buf, " USING INDEX");
- appendStringInfo(&buf, " TABLESPACE %s",
- quote_identifier(get_tablespace_name(tblspc)));
+ if (OidIsValid(tblspc))
+ {
+ if (isConstraint)
+ appendStringInfoString(&buf, " USING INDEX");
+ appendStringInfo(&buf, " TABLESPACE %s",
+ quote_identifier(get_tablespace_name(tblspc)));
+ }
}
/*
@@ -2170,6 +2171,12 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
pfree(options);
}
+ /*
+ * Print the tablespace, unless it's the database default.
+ * This is to help ALTER TABLE usage of this facility,
+ * which needs this behavior to recreate exact catalog
+ * state.
+ */
tblspc = get_rel_tablespace(indexId);
if (OidIsValid(tblspc))
appendStringInfo(&buf, " USING INDEX TABLESPACE %s",
diff --git a/src/include/commands/tablespace.h b/src/include/commands/tablespace.h
index 003c874c228..273e31ccdfb 100644
--- a/src/include/commands/tablespace.h
+++ b/src/include/commands/tablespace.h
@@ -49,7 +49,7 @@ extern Oid AlterTableSpaceOptions(AlterTableSpaceOptionsStmt *stmt);
extern void TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo);
-extern Oid GetDefaultTablespace(char relpersistence);
+extern Oid GetDefaultTablespace(char relpersistence, bool partitioned);
extern void PrepareTempTablespaces(void);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 54cc61751b7..12e9730dd0f 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2154,6 +2154,8 @@ typedef struct Constraint
List *options; /* options from WITH clause */
char *indexname; /* existing index to use; otherwise NULL */
char *indexspace; /* index tablespace; NULL for default */
+ bool reset_default_tblspc; /* reset default_tablespace prior to
+ * creating the index */
/* These could be, but currently are not, used for UNIQUE/PKEY: */
char *access_method; /* index access method; NULL for default */
Node *where_clause; /* partial index predicate */
@@ -2769,6 +2771,8 @@ typedef struct IndexStmt
bool transformed; /* true when transformIndexStmt is finished */
bool concurrent; /* should this be a concurrent index build? */
bool if_not_exists; /* just do nothing if index already exists? */
+ bool reset_default_tblspc; /* reset default_tablespace prior to
+ * executing */
} IndexStmt;
/* ----------------------
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 14ce0e7e04f..791f660ca78 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -65,6 +65,18 @@ SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c
where c.reltablespace = t.oid AND c.relname LIKE 'part%_idx';
\d testschema.part_a_idx
+-- partitioned rels cannot specify the default tablespace. These fail:
+CREATE TABLE testschema.dflt (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE pg_default;
+CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE pg_default) PARTITION BY LIST (a);
+SET default_tablespace TO 'pg_default';
+CREATE TABLE testschema.dflt (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE regress_tblspace;
+CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace) PARTITION BY LIST (a);
+-- but these work:
+CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace) PARTITION BY LIST (a) TABLESPACE regress_tblspace;
+SET default_tablespace TO '';
+CREATE TABLE testschema.dflt2 (a int PRIMARY KEY) PARTITION BY LIST (a);
+DROP TABLE testschema.dflt, testschema.dflt2;
+
-- check that default_tablespace doesn't affect ALTER TABLE index rebuilds
CREATE TABLE testschema.test_default_tab(id bigint) TABLESPACE regress_tblspace;
INSERT INTO testschema.test_default_tab VALUES (1);
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 8ebe08b9b28..fc47e3dabb5 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -102,6 +102,21 @@ Partitioned index "testschema.part_a_idx"
btree, for table "testschema.part"
Tablespace: "regress_tblspace"
+-- partitioned rels cannot specify the default tablespace. These fail:
+CREATE TABLE testschema.dflt (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE pg_default;
+ERROR: cannot specify default tablespace for partitioned relations
+CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE pg_default) PARTITION BY LIST (a);
+ERROR: cannot specify default tablespace for partitioned relation
+SET default_tablespace TO 'pg_default';
+CREATE TABLE testschema.dflt (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE regress_tblspace;
+ERROR: cannot specify default tablespace for partitioned relations
+CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace) PARTITION BY LIST (a);
+ERROR: cannot specify default tablespace for partitioned relations
+-- but these work:
+CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace) PARTITION BY LIST (a) TABLESPACE regress_tblspace;
+SET default_tablespace TO '';
+CREATE TABLE testschema.dflt2 (a int PRIMARY KEY) PARTITION BY LIST (a);
+DROP TABLE testschema.dflt, testschema.dflt2;
-- check that default_tablespace doesn't affect ALTER TABLE index rebuilds
CREATE TABLE testschema.test_default_tab(id bigint) TABLESPACE regress_tblspace;
INSERT INTO testschema.test_default_tab VALUES (1);
