behaviour change - default_tablesapce + partition table
Hi,
Consider the below test:
create tablespace mytbs location '/home/rushabh/mywork/workspace/pg/';
create table test ( a int , b int ) partition by list (a);
set default_tablespace to mytbs;
create table test_p1 partition of test for values in (1);
In the above test, after the setting the default_tablespace I am creating
a partition table and expecting that to get created under "mytbs"
tablespace.
But that is not the case:
postgres@66247=#select relname, reltablespace from pg_class where relname =
'test_p1';
relname | reltablespace
---------+---------------
test_p1 | 0
(1 row)
I noticed the behaviour change for default_tablespace with partition table
with below commit.
commit 87259588d0ab0b8e742e30596afa7ae25caadb18
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu Apr 25 10:20:23 2019 -0400
Fix tablespace inheritance for partitioned rels
Commit ca4103025dfe left a few loose ends. The most important one
(broken pg_dump output) is already fixed by virtue of commit
3b23552ad8bb, but some things remained:
I don't think that new behaviour is intended and if it's an intended change
than need to fix pg_dump as well - other wise lets say,
1) create the above test on v11
2) take a dump
3) restore on v12
will end up partition into wrong tablesapce.
Looking at the commit changes, it seems like at condition when no other
tablespace is specified, we default the tablespace to the parent partitioned
table's.
else if (stmt->partbound)
{
/*
* For partitions, when no other tablespace is specified, we default
* the tablespace to the parent partitioned table's.
*/
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}
But here it doesn't consider the default_tablespace if the parent
partitioned
tablespace is an InvalidOid (which was the care before this commit).
PFA patch to fix the same.
Thanks,
--
Rushabh Lathia
www.EnterpriseDB.com
Attachments:
default_tablespace_for_partition.patchtext/x-patch; charset=US-ASCII; name=default_tablespace_for_partition.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bfcf947..0cc9c0b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -680,6 +680,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
*/
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
+
+ if (!OidIsValid(tablespaceId))
+ tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence, partitioned);
}
else
tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
Agree that this behavior change seems unintentional.
On 2019/05/17 12:40, Rushabh Lathia wrote:
Looking at the commit changes, it seems like at condition when no other
tablespace is specified, we default the tablespace to the parent partitioned
table's.else if (stmt->partbound)
{
/*
* For partitions, when no other tablespace is specified, we default
* the tablespace to the parent partitioned table's.
*/
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}But here it doesn't consider the default_tablespace if the parent
partitioned
tablespace is an InvalidOid (which was the care before this commit).PFA patch to fix the same.
+
+ if (!OidIsValid(tablespaceId))
+ tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
partitioned);
}
else
tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
Why not change it like this instead:
@@ -681,7 +681,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
ownerId,
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}
- else
+
+ if (!OidIsValid(tablespaceId))
tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
partitioned);
Thanks,
Amit
On 2019/05/17 12:40, Rushabh Lathia wrote:
Hi,
Consider the below test:
create tablespace mytbs location '/home/rushabh/mywork/workspace/pg/';
create table test ( a int , b int ) partition by list (a);set default_tablespace to mytbs;
create table test_p1 partition of test for values in (1);In the above test, after the setting the default_tablespace I am creating
a partition table and expecting that to get created under "mytbs"
tablespace.But that is not the case:
postgres@66247=#select relname, reltablespace from pg_class where relname =
'test_p1';
relname | reltablespace
---------+---------------
test_p1 | 0
(1 row)I noticed the behaviour change for default_tablespace with partition table
with below commit.commit 87259588d0ab0b8e742e30596afa7ae25caadb18
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu Apr 25 10:20:23 2019 -0400Fix tablespace inheritance for partitioned rels
Commit ca4103025dfe left a few loose ends. The most important one
(broken pg_dump output) is already fixed by virtue of commit
3b23552ad8bb, but some things remained:I don't think that new behaviour is intended
Should we add this to open items?
Thanks,
Amit
On Fri, May 17, 2019 at 10:30 AM Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
wrote:
Agree that this behavior change seems unintentional.
On 2019/05/17 12:40, Rushabh Lathia wrote:
Looking at the commit changes, it seems like at condition when no other
tablespace is specified, we default the tablespace to the parentpartitioned
table's.
else if (stmt->partbound)
{
/*
* For partitions, when no other tablespace is specified, wedefault
* the tablespace to the parent partitioned table's.
*/
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}But here it doesn't consider the default_tablespace if the parent
partitioned
tablespace is an InvalidOid (which was the care before this commit).PFA patch to fix the same.
+ + if (!OidIsValid(tablespaceId)) + tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence, partitioned); } else tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,Why not change it like this instead:
@@ -681,7 +681,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
ownerId,
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}
- else
+
+ if (!OidIsValid(tablespaceId))
tablespaceId =
GetDefaultTablespace(stmt->relation->relpersistence,
partitioned);
Yes, sure we can do that. Here is the patch for the same.
--
Rushabh Lathia
www.EnterpriseDB.com
Attachments:
default_tablespace_for_partition_v2.patchtext/x-patch; charset=US-ASCII; name=default_tablespace_for_partition_v2.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bfcf947..30a8c84 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -555,7 +555,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
char relname[NAMEDATALEN];
Oid namespaceId;
Oid relationId;
- Oid tablespaceId;
+ Oid tablespaceId = InvalidOid;
Relation rel;
TupleDesc descriptor;
List *inheritOids;
@@ -681,7 +681,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}
- else
+
+ if (!OidIsValid(tablespaceId))
tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
partitioned);
On 2019/05/20 13:42, Rushabh Lathia wrote:
On Fri, May 17, 2019 at 10:30 AM Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Why not change it like this instead:
@@ -681,7 +681,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
ownerId,
Assert(list_length(inheritOids) == 1);
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}
- else
+
+ if (!OidIsValid(tablespaceId))
tablespaceId =
GetDefaultTablespace(stmt->relation->relpersistence,
partitioned);Yes, sure we can do that. Here is the patch for the same.
Thanks Rushabh.
Regards,
Amit
On 2019-May-20, Amit Langote wrote:
Should we add this to open items?
Yeah. I'm AFK this week, but can handle it afterwards. The fix already
missed beta1, so I don't think there's a problem with taking a little
bit longer.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019/05/21 11:29, Alvaro Herrera wrote:
On 2019-May-20, Amit Langote wrote:
Should we add this to open items?
Yeah. I'm AFK this week, but can handle it afterwards. The fix already
missed beta1, so I don't think there's a problem with taking a little
bit longer.
OK, added.
Thanks,
Amit
On 2019-May-20, Alvaro Herrera wrote:
On 2019-May-20, Amit Langote wrote:
Should we add this to open items?
Yeah. I'm AFK this week, but can handle it afterwards. The fix already
missed beta1, so I don't think there's a problem with taking a little
bit longer.
Here's my proposed patch. I changed/expanded the tests a little bit to
ensure more complete coverage.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
tablespace-partitioned.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e13b96d252..b3095bd937 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -682,6 +682,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
tablespaceId = get_rel_tablespace(linitial_oid(inheritOids));
}
else
+ tablespaceId = InvalidOid;
+
+ if (!OidIsValid(tablespaceId)) /* note no "else" */
tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
partitioned);
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 78199eb6f5..8f012fcc33 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -44,16 +44,38 @@ CREATE INDEX foo_idx on testschema.foo(i) TABLESPACE regress_tblspace;
SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c
where c.reltablespace = t.oid AND c.relname = 'foo_idx';
+--
-- partitioned table
+--
CREATE TABLE testschema.part (a int) PARTITION BY LIST (a);
-CREATE TABLE testschema.part12 PARTITION OF testschema.part FOR VALUES IN(1,2) PARTITION BY LIST (a) TABLESPACE regress_tblspace;
-CREATE TABLE testschema.part12_1 PARTITION OF testschema.part12 FOR VALUES IN (1);
-ALTER TABLE testschema.part12 SET TABLESPACE pg_default;
-CREATE TABLE testschema.part12_2 PARTITION OF testschema.part12 FOR VALUES IN (2);
--- Ensure part12_1 defaulted to regress_tblspace and part12_2 defaulted to pg_default.
+SET default_tablespace TO pg_global;
+CREATE TABLE testschema.part_1 PARTITION OF testschema.part FOR VALUES IN (1);
+RESET default_tablespace;
+CREATE TABLE testschema.part_1 PARTITION OF testschema.part FOR VALUES IN (1);
+SET default_tablespace TO regress_tblspace;
+CREATE TABLE testschema.part_2 PARTITION OF testschema.part FOR VALUES IN (2);
+SET default_tablespace TO pg_global;
+CREATE TABLE testschema.part_3 PARTITION OF testschema.part FOR VALUES IN (3);
+ALTER TABLE testschema.part SET TABLESPACE regress_tblspace;
+CREATE TABLE testschema.part_3 PARTITION OF testschema.part FOR VALUES IN (3);
+CREATE TABLE testschema.part_4 PARTITION OF testschema.part FOR VALUES IN (4)
+ TABLESPACE pg_default;
+CREATE TABLE testschema.part_56 PARTITION OF testschema.part FOR VALUES IN (5, 6)
+ PARTITION BY LIST (a);
+ALTER TABLE testschema.part SET TABLESPACE pg_default;
+CREATE TABLE testschema.part_78 PARTITION OF testschema.part FOR VALUES IN (7, 8)
+ PARTITION BY LIST (a);
+CREATE TABLE testschema.part_910 PARTITION OF testschema.part FOR VALUES IN (9, 10)
+ PARTITION BY LIST (a) TABLESPACE regress_tblspace;
+RESET default_tablespace;
+CREATE TABLE testschema.part_78 PARTITION OF testschema.part FOR VALUES IN (7, 8)
+ PARTITION BY LIST (a);
+
SELECT relname, spcname FROM pg_catalog.pg_class c
+ JOIN pg_catalog.pg_namespace n ON (c.relnamespace = n.oid)
LEFT JOIN pg_catalog.pg_tablespace t ON c.reltablespace = t.oid
- where c.relname LIKE 'part%' order by relname;
+ where c.relname LIKE 'part%' AND n.nspname = 'testschema' order by relname;
+RESET default_tablespace;
DROP TABLE testschema.part;
-- partitioned index
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 7625374d46..2ea68cabb0 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -61,24 +61,52 @@ SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c
foo_idx | regress_tblspace
(1 row)
+--
-- partitioned table
+--
CREATE TABLE testschema.part (a int) PARTITION BY LIST (a);
-CREATE TABLE testschema.part12 PARTITION OF testschema.part FOR VALUES IN(1,2) PARTITION BY LIST (a) TABLESPACE regress_tblspace;
-CREATE TABLE testschema.part12_1 PARTITION OF testschema.part12 FOR VALUES IN (1);
-ALTER TABLE testschema.part12 SET TABLESPACE pg_default;
-CREATE TABLE testschema.part12_2 PARTITION OF testschema.part12 FOR VALUES IN (2);
--- Ensure part12_1 defaulted to regress_tblspace and part12_2 defaulted to pg_default.
+SET default_tablespace TO pg_global;
+CREATE TABLE testschema.part_1 PARTITION OF testschema.part FOR VALUES IN (1);
+ERROR: only shared relations can be placed in pg_global tablespace
+RESET default_tablespace;
+CREATE TABLE testschema.part_1 PARTITION OF testschema.part FOR VALUES IN (1);
+SET default_tablespace TO regress_tblspace;
+CREATE TABLE testschema.part_2 PARTITION OF testschema.part FOR VALUES IN (2);
+SET default_tablespace TO pg_global;
+CREATE TABLE testschema.part_3 PARTITION OF testschema.part FOR VALUES IN (3);
+ERROR: only shared relations can be placed in pg_global tablespace
+ALTER TABLE testschema.part SET TABLESPACE regress_tblspace;
+CREATE TABLE testschema.part_3 PARTITION OF testschema.part FOR VALUES IN (3);
+CREATE TABLE testschema.part_4 PARTITION OF testschema.part FOR VALUES IN (4)
+ TABLESPACE pg_default;
+CREATE TABLE testschema.part_56 PARTITION OF testschema.part FOR VALUES IN (5, 6)
+ PARTITION BY LIST (a);
+ALTER TABLE testschema.part SET TABLESPACE pg_default;
+CREATE TABLE testschema.part_78 PARTITION OF testschema.part FOR VALUES IN (7, 8)
+ PARTITION BY LIST (a);
+ERROR: only shared relations can be placed in pg_global tablespace
+CREATE TABLE testschema.part_910 PARTITION OF testschema.part FOR VALUES IN (9, 10)
+ PARTITION BY LIST (a) TABLESPACE regress_tblspace;
+RESET default_tablespace;
+CREATE TABLE testschema.part_78 PARTITION OF testschema.part FOR VALUES IN (7, 8)
+ PARTITION BY LIST (a);
SELECT relname, spcname FROM pg_catalog.pg_class c
+ JOIN pg_catalog.pg_namespace n ON (c.relnamespace = n.oid)
LEFT JOIN pg_catalog.pg_tablespace t ON c.reltablespace = t.oid
- where c.relname LIKE 'part%' order by relname;
+ where c.relname LIKE 'part%' AND n.nspname = 'testschema' order by relname;
relname | spcname
----------+------------------
part |
- part12 |
- part12_1 | regress_tblspace
- part12_2 |
-(4 rows)
+ part_1 |
+ part_2 | regress_tblspace
+ part_3 | regress_tblspace
+ part_4 |
+ part_56 | regress_tblspace
+ part_78 |
+ part_910 | regress_tblspace
+(8 rows)
+RESET default_tablespace;
DROP TABLE testschema.part;
-- partitioned index
CREATE TABLE testschema.part (a int) PARTITION BY LIST (a);
On 2019-Jun-06, Alvaro Herrera wrote:
Here's my proposed patch. I changed/expanded the tests a little bit to
ensure more complete coverage.
Well, revise the comments a little bit.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
comments.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3095bd937..2a72c1b501 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -660,8 +660,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
}
/*
- * Select tablespace to use. If not specified, use default tablespace
- * (which may in turn default to database's default).
+ * Select tablespace to use: an explicitly indicated one, or (in the case
+ * of a partitioned table) the parent's, if it has one.
*/
if (stmt->tablespacename)
{
@@ -684,7 +684,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
else
tablespaceId = InvalidOid;
- if (!OidIsValid(tablespaceId)) /* note no "else" */
+ /* still nothing? use the default */
+ if (!OidIsValid(tablespaceId))
tablespaceId = GetDefaultTablespace(stmt->relation->relpersistence,
partitioned);
Hi Alvaro,
On Fri, Jun 7, 2019 at 8:02 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Jun-06, Alvaro Herrera wrote:
Here's my proposed patch. I changed/expanded the tests a little bit to
ensure more complete coverage.Well, revise the comments a little bit.
Thanks for adding the tests. Looks good.
Regards,
Amit
Hi Amit, Rushabh,
On 2019-Jun-07, Amit Langote wrote:
Thanks for adding the tests. Looks good.
Thanks Amit, pushed now. Marking open item as done.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services