Horton

Thread

Commits

  1. Fix default_tablespace usage for partitioned tables

  2. Fix tablespace inheritance for partitioned rels

  3. Make pg_dump emit ATTACH PARTITION instead of PARTITION OF

  4. Fix tablespace handling for partitioned tables

  1. behaviour change - default_tablesapce + partition table

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2019-05-17T03:40:10Z

    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

  2. Re: behaviour change - default_tablesapce + partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-05-17T05:00:22Z

    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

  3. Re: behaviour change - default_tablesapce + partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-05-20T04:30:30Z

    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

  4. Re: behaviour change - default_tablesapce + partition table

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2019-05-20T04:42:40Z

    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

  5. Re: behaviour change - default_tablesapce + partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-05-20T04:45:10Z

    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

  6. Re: behaviour change - default_tablesapce + partition table

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-21T02:29:22Z

    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

  7. Re: behaviour change - default_tablesapce + partition table

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-05-21T03:00:36Z

    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

  8. Re: behaviour change - default_tablesapce + partition table

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-06T22:52:24Z

    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

  9. Re: behaviour change - default_tablesapce + partition table

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-06T23:02:42Z

    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

  10. Re: behaviour change - default_tablesapce + partition table

    Amit Langote <amitlangote09@gmail.com> — 2019-06-07T00:51:23Z

    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

  11. Re: behaviour change - default_tablesapce + partition table

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-07T05:10:07Z

    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