Обсуждение: 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 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