behaviour change - default_tablesapce + partition table

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 -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

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 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);

Yes, sure we can do that. Here is the patch for the same.

--
Rushabh Lathia
www.EnterpriseDB.com

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

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

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