Re: [HACKERS] dropping partitioned tables without CASCADE
Somehow the earlier patches missed qualifying pg_get_expr() by
pg_catalog. Fixed it along with annotating the partitioned partition
as ", PARTITIONED".
On Fri, Nov 3, 2017 at 6:09 PM, Alvaro Herrera wrote:
>>
>> Right now, we could do that if we order the list by bound expression;
>> lexically DEFAULT would come before FOR VALUES ... . But that's not
>> future-safe; we may have a bound expression starting with A, B or C.
>> Beyond that it really gets tricky to order the partitions by bounds.
>
> I was just thinking in changing the query to be "order by
> is_the_default_partition, partition_name" instead of just "order by
> partition_name". Sorting by bounds rather than name (a feature whose
> worth should definitely be discussed separately IMV) sounds a lot more
> complicated.
Right now we don't have a catalog column or a SQL function which can
tell whether a given partition is default partition based on the
partition bounds or otherwise. That's what it seemed when you
suggested ordering by "is_the_default_partition". Instead I have
ordered the partitions by pg_catalog.pg_get_expr(...) = 'DEFAULT'. We
can introduce a SQL function which takes child and parent oids and
return true if it's default partition and use that here, but that
seems more than what you are suggesting here. I have added that as a
separate patch.
If we tackle the problem of listing partitions by their bounds
somehow, DEFAULT partition listing would be tackled anyway. So, may be
we should leave it outside the scope of this patch.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
From 398003b2d5f6b54e6cdd8542f653786987ef3bfe Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Mon, 4 Sep 2017 09:56:41 +0530
Subject: [PATCH 1/2] Improve \d+ output of a partitioned table
While displaying partitions in \d+ output of a partitioned table
annotate the partitioned partitions as "PARTITIONED".
For a partitioned table show the number of partitions even if it's 0.
Ashutosh Bapat and Amit Langote.
---
src/bin/psql/describe.c| 34 +++-
src/test/regress/expected/create_table.out | 13 +++
src/test/regress/expected/foreign_data.out |3 +++
src/test/regress/expected/insert.out | 17 ++
src/test/regress/sql/create_table.sql |2 +-
src/test/regress/sql/insert.sql|4
6 files changed, 61 insertions(+), 12 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index b7b978a..44c5089 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2870,7 +2870,9 @@ describeOneTableDetails(const char *schemaname,
/* print child tables (with additional info if partitions) */
if (pset.sversion >= 10)
printfPQExpBuffer(&buf,
- "SELECT c.oid::pg_catalog.regclass, pg_catalog.pg_get_expr(c.relpartbound, c.oid)"
+ "SELECT c.oid::pg_catalog.regclass,"
+ " pg_catalog.pg_get_expr(c.relpartbound, c.oid),"
+ " c.relkind"
" FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i"
" WHERE c.oid=i.inhrelid AND i.inhparent = '%s'"
" ORDER BY c.oid::pg_catalog.regclass::pg_catalog.text;", oid);
@@ -2893,7 +2895,18 @@ describeOneTableDetails(const char *schemaname,
else
tuples = PQntuples(result);
- if (!verbose)
+ /*
+ * For a partitioned table with no partitions, always print the number
+ * of partitions as zero, even when verbose output is expected.
+ * Otherwise, we will not print "Partitions" section for a partitioned
+ * table without any partitions.
+ */
+ if (tableinfo.relkind == RELKIND_PARTITIONED_TABLE && tuples == 0)
+ {
+ printfPQExpBuffer(&buf, _("Number of partitions: %d"), tuples);
+ printTableAddFooter(&cont, buf.data);
+ }
+ else if (!verbose)
{
/* print the number of child tables, if any */
if (tuples > 0)
@@ -2925,12 +2938,21 @@ describeOneTableDetails(const char *schemaname,
}
else
{
+ char *partitioned_note;
+
+ if (*PQgetvalue(result, i, 2) == RELKIND_PARTITIONED_TABLE)
+ partitioned_note = ", PARTITIONED";
+ else
+ partitioned_note = "";
+
if (i == 0)
- printfPQExpBuffer(&buf, "%s: %s %s",
- ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%s: %s %s%s",
+ ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+ partitioned_note);
else
- printfPQExpBuffer(&buf, "%*s %s %s",
- ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%*s %s %s%s",
+ ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+ partitioned_note);
}
if (i < tuples - 1)
appendPQExpBufferChar(&buf, ',');
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 60ab28a..ac6f576 100644
--- a/src/test/regress/e
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/11/03 21:39, Alvaro Herrera wrote: > Ashutosh Bapat wrote: >> On Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera >> wrote: > >>> I think adding "is partitioned" at end of line isn't good; looks like a >>> phrase but isn't translatable. Maybe add keyword PARTITIONED instead? >> >> In that case may be we should separate bounds and "PARTITIONED" with a >> ",". "part_default DEFAULT, PARTITIONED" would read better than >> "part_default DEFAULT PARTITIONED"? > > Hmm, I vote +0.5 for the comma. Me too. >>> Is it possible to put it at either start or end of the list? >> >> Right now, we could do that if we order the list by bound expression; >> lexically DEFAULT would come before FOR VALUES ... . But that's not >> future-safe; we may have a bound expression starting with A, B or C. >> Beyond that it really gets tricky to order the partitions by bounds. > > I was just thinking in changing the query to be "order by > is_the_default_partition, partition_name" instead of just "order by > partition_name". Sorting by bounds rather than name (a feature whose > worth should definitely be discussed separately IMV) sounds a lot more > complicated. Yeah, it sounds like a desirable feature, but as you both say, should be discussed separately. Since the facility to order partitions in the bound order is internal to the server yet, we'd need some new server-side functionality to expose the same with sane SQL-callable interface, which clearly needs its own separate discussion. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
Ashutosh Bapat wrote: > On Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera > wrote: > > I think adding "is partitioned" at end of line isn't good; looks like a > > phrase but isn't translatable. Maybe add keyword PARTITIONED instead? > > In that case may be we should separate bounds and "PARTITIONED" with a > ",". "part_default DEFAULT, PARTITIONED" would read better than > "part_default DEFAULT PARTITIONED"? Hmm, I vote +0.5 for the comma. > > Having the DEFAULT partition show up in the middle of the list is weird. > > Agreed. But that's true even without this patch. Yes. > > Is it possible to put it at either start or end of the list? > > Right now, we could do that if we order the list by bound expression; > lexically DEFAULT would come before FOR VALUES ... . But that's not > future-safe; we may have a bound expression starting with A, B or C. > Beyond that it really gets tricky to order the partitions by bounds. I was just thinking in changing the query to be "order by is_the_default_partition, partition_name" instead of just "order by partition_name". Sorting by bounds rather than name (a feature whose worth should definitely be discussed separately IMV) sounds a lot more complicated. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera wrote: > Amit Langote wrote: >> On 2017/09/06 19:14, Amit Langote wrote: >> > On 2017/09/06 18:46, Rushabh Lathia wrote: >> >> Okay, I have marked this as ready for committer. >> > >> > Thanks Ashutosh and Rushabh for rebasing and improving the patch. Looks >> > good to me too. >> >> Patch needed to be rebased after the default partitions patch went in, so >> done. Per build status on http://commitfest.cputube.org :) > > I think adding "is partitioned" at end of line isn't good; looks like a > phrase but isn't translatable. Maybe add keyword PARTITIONED instead? In that case may be we should separate bounds and "PARTITIONED" with a ",". "part_default DEFAULT, PARTITIONED" would read better than "part_default DEFAULT PARTITIONED"? > > Having the DEFAULT partition show up in the middle of the list is weird. Agreed. But that's true even without this patch. > Is it possible to put it at either start or end of the list? > Right now, we could do that if we order the list by bound expression; lexically DEFAULT would come before FOR VALUES ... . But that's not future-safe; we may have a bound expression starting with A, B or C. Beyond that it really gets tricky to order the partitions by bounds. The goal of this patch is to mark the partitioned partitions as such and show the number of partitions. While your suggestion is a valid request, it's kind of beyond the scope of this patch. Someone might want to extend this request and say that partitions should be listed in the order of their bounds (I do feel that we should do some effort in that direction). But I am not sure whether it should be done in this patch. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
Amit Langote wrote: > On 2017/09/06 19:14, Amit Langote wrote: > > On 2017/09/06 18:46, Rushabh Lathia wrote: > >> Okay, I have marked this as ready for committer. > > > > Thanks Ashutosh and Rushabh for rebasing and improving the patch. Looks > > good to me too. > > Patch needed to be rebased after the default partitions patch went in, so > done. Per build status on http://commitfest.cputube.org :) I think adding "is partitioned" at end of line isn't good; looks like a phrase but isn't translatable. Maybe add keyword PARTITIONED instead? Having the DEFAULT partition show up in the middle of the list is weird. Is it possible to put it at either start or end of the list? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
Thanks Amit for taking care of this. On Wed, Sep 13, 2017 at 6:31 AM, Amit Langote wrote: > On 2017/09/06 19:14, Amit Langote wrote: >> On 2017/09/06 18:46, Rushabh Lathia wrote: >>> Okay, I have marked this as ready for committer. >> >> Thanks Ashutosh and Rushabh for rebasing and improving the patch. Looks >> good to me too. > > Patch needed to be rebased after the default partitions patch went in, so > done. Per build status on http://commitfest.cputube.org :) > > Thanks, > Amit -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/09/06 19:14, Amit Langote wrote:
> On 2017/09/06 18:46, Rushabh Lathia wrote:
>> Okay, I have marked this as ready for committer.
>
> Thanks Ashutosh and Rushabh for rebasing and improving the patch. Looks
> good to me too.
Patch needed to be rebased after the default partitions patch went in, so
done. Per build status on http://commitfest.cputube.org :)
Thanks,
Amit
From 0ac21ff604b5dccf818f9d69c945ff845d1771bf Mon Sep 17 00:00:00 2001
From: amit
Date: Wed, 13 Sep 2017 09:56:34 +0900
Subject: [PATCH] Some enhancments for \d+ output of partitioned tables
---
src/bin/psql/describe.c| 32 --
src/test/regress/expected/create_table.out | 13 +++-
src/test/regress/expected/foreign_data.out | 3 +++
src/test/regress/expected/insert.out | 17
src/test/regress/sql/create_table.sql | 2 +-
src/test/regress/sql/insert.sql| 4
6 files changed, 59 insertions(+), 12 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index d22ec68431..855e6870e9 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2831,7 +2831,7 @@ describeOneTableDetails(const char *schemaname,
/* print child tables (with additional info if partitions) */
if (pset.sversion >= 10)
printfPQExpBuffer(&buf,
- "SELECT
c.oid::pg_catalog.regclass, pg_catalog.pg_get_expr(c.relpartbound, c.oid)"
+ "SELECT
c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid), c.relkind"
" FROM
pg_catalog.pg_class c, pg_catalog.pg_inherits i"
" WHERE
c.oid=i.inhrelid AND i.inhparent = '%s'"
" ORDER BY
c.oid::pg_catalog.regclass::pg_catalog.text;", oid);
@@ -2854,7 +2854,18 @@ describeOneTableDetails(const char *schemaname,
else
tuples = PQntuples(result);
- if (!verbose)
+ /*
+* For a partitioned table with no partitions, always print the
number
+* of partitions as zero, even when verbose output is expected.
+* Otherwise, we will not print "Partitions" section for a
partitioned
+* table without any partitions.
+*/
+ if (tableinfo.relkind == RELKIND_PARTITIONED_TABLE && tuples ==
0)
+ {
+ printfPQExpBuffer(&buf, _("Number of partitions: %d"),
tuples);
+ printTableAddFooter(&cont, buf.data);
+ }
+ else if (!verbose)
{
/* print the number of child tables, if any */
if (tuples > 0)
@@ -2886,12 +2897,21 @@ describeOneTableDetails(const char *schemaname,
}
else
{
+ char *partitioned_note;
+
+ if (*(PQgetvalue(result, i, 2)) ==
RELKIND_PARTITIONED_TABLE)
+ partitioned_note = " is
partitioned";
+ else
+ partitioned_note = "";
+
if (i == 0)
- printfPQExpBuffer(&buf, "%s: %s
%s",
-
ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%s: %s
%s%s",
+
ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+
partitioned_note);
else
- printfPQExpBuffer(&buf, "%*s
%s %s",
-
ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%*s
%s %s%s",
+
ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+
partitioned_note);
}
if (i < tuples - 1)
appendPQExpBufferChar(&buf, ',');
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/ex
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/09/06 18:46, Rushabh Lathia wrote: > Okay, I have marked this as ready for committer. Thanks Ashutosh and Rushabh for rebasing and improving the patch. Looks good to me too. Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
Okay, I have marked this as ready for committer. Thanks, On Wed, Sep 6, 2017 at 2:50 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Wed, Sep 6, 2017 at 1:06 PM, Rushabh Lathia > wrote: > > > > 2) Add partition to the foo; > > > > create table foo_p1 partition of foo for values in (1, 2, 3) partition by > > list (b); > > > > postgres=# \d foo > > Table "public.foo" > > Column | Type | Collation | Nullable | Default > > +-+---+--+- > > a | integer | | | > > b | integer | | | > > Partition key: LIST (a) > > Number of partitions: 1 (Use \d+ to list them.) > > > > postgres=# \d+ foo > > Table "public.foo" > > Column | Type | Collation | Nullable | Default | Storage | Stats > target > > | Description > > +-+---+--+-+ > -+--+- > > a | integer | | | | plain | > > | > > b | integer | | | | plain | > > | > > Partition key: LIST (a) > > Partitions: foo_p1 FOR VALUES IN (1, 2, 3) has partitions > > > > Above verbose output for foo says, foo_p1 "has partitions". But if I do > > > > postgres=# \d foo_p1 > >Table "public.foo_p1" > > Column | Type | Collation | Nullable | Default > > +-+---+--+- > > a | integer | | | > > b | integer | | | > > Partition of: foo FOR VALUES IN (1, 2, 3) > > Partition key: LIST (b) > > Number of partitions: 0 > > > > it tell "Number of partitions: 0". > > > > I feel like information is conflicting with each other. AFAIU, idea about > > adding > > "has partitions" was to let know that it's a partitioned table. So can > you > > directly > > add the "is partitioned" in place of "has partitions"? > > > > Did those change in the attached patch and update regression expected > > output. > > > > Looks better. > > > Also run pgindent on the patch. > > > > Thanks for the changes. The patch looks good to me. > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > -- Rushabh Lathia
Re: [HACKERS] dropping partitioned tables without CASCADE
On Wed, Sep 6, 2017 at 1:06 PM, Rushabh Lathia wrote: > > 2) Add partition to the foo; > > create table foo_p1 partition of foo for values in (1, 2, 3) partition by > list (b); > > postgres=# \d foo > Table "public.foo" > Column | Type | Collation | Nullable | Default > +-+---+--+- > a | integer | | | > b | integer | | | > Partition key: LIST (a) > Number of partitions: 1 (Use \d+ to list them.) > > postgres=# \d+ foo > Table "public.foo" > Column | Type | Collation | Nullable | Default | Storage | Stats target > | Description > +-+---+--+-+-+--+- > a | integer | | | | plain | > | > b | integer | | | | plain | > | > Partition key: LIST (a) > Partitions: foo_p1 FOR VALUES IN (1, 2, 3) has partitions > > Above verbose output for foo says, foo_p1 "has partitions". But if I do > > postgres=# \d foo_p1 >Table "public.foo_p1" > Column | Type | Collation | Nullable | Default > +-+---+--+- > a | integer | | | > b | integer | | | > Partition of: foo FOR VALUES IN (1, 2, 3) > Partition key: LIST (b) > Number of partitions: 0 > > it tell "Number of partitions: 0". > > I feel like information is conflicting with each other. AFAIU, idea about > adding > "has partitions" was to let know that it's a partitioned table. So can you > directly > add the "is partitioned" in place of "has partitions"? > > Did those change in the attached patch and update regression expected > output. > Looks better. > Also run pgindent on the patch. > Thanks for the changes. The patch looks good to me. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
I picked this patch for review and started looking at the implementation
details.
Consider the below test:
1)
postgres=# create table foo (a int, b int) partition by list (a);
CREATE TABLE
postgres=# \d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
+-+---+--+-
a | integer | | |
b | integer | | |
Partition key: LIST (a)
Number of partitions: 0
postgres=# \d+ foo
Table "public.foo"
Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
a | integer | | | | plain |
|
b | integer | | | | plain |
|
Partition key: LIST (a)
Number of partitions: 0
In the above case, verbose as well as normal output give information
about number of partitions.
2) Add partition to the foo;
create table foo_p1 partition of foo for values in (1, 2, 3) partition by
list (b);
postgres=# \d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
+-+---+--+-
a | integer | | |
b | integer | | |
Partition key: LIST (a)
*Number of partitions: 1 (Use \d+ to list them.)*
postgres=# \d+ foo
Table "public.foo"
Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
a | integer | | | | plain |
|
b | integer | | | | plain |
|
Partition key: LIST (a)
*Partitions: foo_p1 FOR VALUES IN (1, 2, 3) has partitions*
Above verbose output for foo says, foo_p1 "has partitions". But if I do
postgres=# \d foo_p1
Table "public.foo_p1"
Column | Type | Collation | Nullable | Default
+-+---+--+-
a | integer | | |
b | integer | | |
Partition of: foo FOR VALUES IN (1, 2, 3)
Partition key: LIST (b)
*Number of partitions: 0*
it tell "Number of partitions: 0".
I feel like information is conflicting with each other. AFAIU, idea about
adding
"has partitions" was to let know that it's a partitioned table. So can you
directly
add the "is partitioned" in place of "has partitions"?
Did those change in the attached patch and update regression expected
output.
Also run pgindent on the patch.
Thanks,
On Mon, Sep 4, 2017 at 6:22 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
> On Mon, Sep 4, 2017 at 3:48 PM, Alvaro Herrera
> wrote:
> >
> > if (tuples > 0)
> > {
> > - if (tableinfo.relkind !=
> RELKIND_PARTITIONED_TABLE)
> > - printfPQExpBuffer(&buf,
> _("Number of child tables: %d (Use \\d+ to list them.)"), tuples);
> > - else
> > - printfPQExpBuffer(&buf,
> _("Number of partitions: %d (Use \\d+ to list them.)"), tuples);
> > + printfPQExpBuffer(&buf, _("Number of %s:
> %d (Use \\d+ to list them.)"), ct, tuples);
> > printTableAddFooter(&cont, buf.data);
> > }
> >
> > Please don't do this, because it breaks translatability. I think the
> > original is fine.
> >
>
> We have used this style in the "else" case of if (!verbose). So, I
> just copied it. I have removed that change in the attached patch.
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
...
Rushabh Lathia
www.EnterpriseDB.com
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f6049cc..ab9a637 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2828,7 +2828,7 @@ describeOneTableDetails(const char *schemaname,
/* print child tables (with additional info if partitions) */
if (pset.sversion >= 10)
printfPQExpBuffer(&buf,
- "SELECT c.oid::pg_catalog.regclass, pg_catalog.pg_get_expr(c.relpartbound, c.oid)"
+ "SELECT c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid), c.relkind"
" FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i"
" WHERE c.oid=i.inhrelid AND i.inhparent = '%s'"
" ORDER BY c.oid::pg_catalog.regclass::pg_catalog.text;", oid);
@@ -2851,7 +2851,18 @@ describeOneTableDetails(const char *schemaname,
else
tuples = PQntuples(result);
- if (!verbose)
+ /*
+ * F
Re: [HACKERS] dropping partitioned tables without CASCADE
On Mon, Sep 4, 2017 at 3:48 PM, Alvaro Herrera wrote:
>
> if (tuples > 0)
> {
> - if (tableinfo.relkind !=
> RELKIND_PARTITIONED_TABLE)
> - printfPQExpBuffer(&buf, _("Number of
> child tables: %d (Use \\d+ to list them.)"), tuples);
> - else
> - printfPQExpBuffer(&buf, _("Number of
> partitions: %d (Use \\d+ to list them.)"), tuples);
> + printfPQExpBuffer(&buf, _("Number of %s: %d
> (Use \\d+ to list them.)"), ct, tuples);
> printTableAddFooter(&cont, buf.data);
> }
>
> Please don't do this, because it breaks translatability. I think the
> original is fine.
>
We have used this style in the "else" case of if (!verbose). So, I
just copied it. I have removed that change in the attached patch.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
From c0a153f0535c4d1dca637996d4cd5e6f62c11afe Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Mon, 4 Sep 2017 09:56:41 +0530
Subject: [PATCH] Improve \d+ output of a partitioned table
While displaying partitions in \d+ output of a partitioned table
annotate the partitioned partitions as "has partitions".
For a partitioned table show the number of partitions even if it's 0.
Ashutosh Bapat and Amit Langote.
---
src/bin/psql/describe.c| 32 ++--
src/test/regress/expected/create_table.out | 13 ++-
src/test/regress/expected/foreign_data.out |3 +++
src/test/regress/expected/insert.out | 15 +
src/test/regress/sql/create_table.sql |2 +-
src/test/regress/sql/insert.sql|4
6 files changed, 57 insertions(+), 12 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f6049cc..5adf5a4 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2828,7 +2828,7 @@ describeOneTableDetails(const char *schemaname,
/* print child tables (with additional info if partitions) */
if (pset.sversion >= 10)
printfPQExpBuffer(&buf,
- "SELECT c.oid::pg_catalog.regclass, pg_catalog.pg_get_expr(c.relpartbound, c.oid)"
+ "SELECT c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid), c.relkind"
" FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i"
" WHERE c.oid=i.inhrelid AND i.inhparent = '%s'"
" ORDER BY c.oid::pg_catalog.regclass::pg_catalog.text;", oid);
@@ -2851,7 +2851,18 @@ describeOneTableDetails(const char *schemaname,
else
tuples = PQntuples(result);
- if (!verbose)
+ /*
+ * For a partitioned table with no partitions, always print the number
+ * of partitions as zero, even when verbose output is expected.
+ * Otherwise, we will not print "Partitions" section for a partitioned
+ * table without any partitions.
+ */
+ if (tableinfo.relkind == RELKIND_PARTITIONED_TABLE && tuples == 0)
+ {
+ printfPQExpBuffer(&buf, _("Number of partitions: %d"), tuples);
+ printTableAddFooter(&cont, buf.data);
+ }
+ else if (!verbose)
{
/* print the number of child tables, if any */
if (tuples > 0)
@@ -2883,12 +2894,21 @@ describeOneTableDetails(const char *schemaname,
}
else
{
+ char *partitioned_note;
+
+ if (*PQgetvalue(result, i, 2) == RELKIND_PARTITIONED_TABLE)
+ partitioned_note = " has partitions";
+ else
+ partitioned_note = "";
+
if (i == 0)
- printfPQExpBuffer(&buf, "%s: %s %s",
- ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%s: %s %s%s",
+ ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+ partitioned_note);
else
- printfPQExpBuffer(&buf, "%*s %s %s",
- ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%*s %s %s%s",
+ ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+ partitioned_note);
}
if (i < tuples - 1)
appendPQExpBufferChar(&buf, ',');
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index babda89..a35d19e 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -428,13 +428,15 @@ ERROR: cannot inherit from partitioned table "partitioned2"
c | text| | |
d | text| | |
Partition key: RANGE (a oid_ops, plusone(b), c, d COLLATE "C")
+Number of partitions: 0
-\d partitioned2
-Table "public.partitioned2"
- Column | Type | Collation | Nullable | Default
-+-+---+--+-
- a | integer | | |
+\d+ partitioned2
+ Tab
Re: [HACKERS] dropping partitioned tables without CASCADE
if (tuples > 0)
{
- if (tableinfo.relkind !=
RELKIND_PARTITIONED_TABLE)
- printfPQExpBuffer(&buf, _("Number of
child tables: %d (Use \\d+ to list them.)"), tuples);
- else
- printfPQExpBuffer(&buf, _("Number of
partitions: %d (Use \\d+ to list them.)"), tuples);
+ printfPQExpBuffer(&buf, _("Number of %s: %d
(Use \\d+ to list them.)"), ct, tuples);
printTableAddFooter(&cont, buf.data);
}
Please don't do this, because it breaks translatability. I think the
original is fine.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On Mon, Sep 4, 2017 at 10:34 AM, Amit Langote wrote: > Hi Ashutosh, > > On 2017/09/04 13:51, Ashutosh Bapat wrote: >> Hi, >> Thomas's application to track patches told me that this patch needs >> rebase. It also required some changes to the code. Here's the updated >> version. I have squashed those two patches together. > > Thanks for the rebased patch. > > Would it make sense to post them in a new thread? When this message > popped into my inbox in a thread titled "dropping partitioned tables > without CASCADE", I wondered for a second if there is still something left > to be done about that. :) That would be good. We will have to update the thread in commitfest entry though. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
Hi Ashutosh, On 2017/09/04 13:51, Ashutosh Bapat wrote: > Hi, > Thomas's application to track patches told me that this patch needs > rebase. It also required some changes to the code. Here's the updated > version. I have squashed those two patches together. Thanks for the rebased patch. Would it make sense to post them in a new thread? When this message popped into my inbox in a thread titled "dropping partitioned tables without CASCADE", I wondered for a second if there is still something left to be done about that. :) Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
Hi,
Thomas's application to track patches told me that this patch needs
rebase. It also required some changes to the code. Here's the updated
version. I have squashed those two patches together.
On Tue, Mar 14, 2017 at 6:35 PM, Ashutosh Bapat
wrote:
> Added this to 2017/7 commitfest to keep a track of it.
>
> On Wed, Mar 8, 2017 at 3:39 PM, Amit Langote
> wrote:
>> On 2017/03/08 18:27, Ashutosh Bapat wrote:
About the other statement you changed, I just realized that we should
perhaps do one more thing. Show the Number of partitions, even if it's 0.
In case of inheritance, the parent table stands on its own when there are
no child tables, but a partitioned table doesn't in the same sense. I
tried to implement that in attached patch 0002. Example below:
create table p (a int) partition by list (a);
\d p
Partition key: LIST (a)
Number of partitions: 0
\d+ p
Partition key: LIST (a)
Number of partitions: 0
create table p1 partition of p for values in (1);
\d p
Partition key: LIST (a)
Number of partitions: 1 (Use \d+ to list them.)
\d+ p
Partition key: LIST (a)
Partitions: p1 FOR VALUES IN (1)
>>>
>>> I liked that. PFA 0002 updated. I changed one of \d output to \d+ to
>>> better test partitioned tables without partitions in verbose and
>>> non-verbose mode. Also, refactored the your code to have less number
>>> of conditions. Please let me know if it looks good.
>>
>> Thanks, looks good.
>>
>> Regards,
>> Amit
>>
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
From 5e5af858931f11b86e8b535331849e58d9a08281 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Mon, 4 Sep 2017 09:56:41 +0530
Subject: [PATCH] Improve \d+ output of a partitioned table
While displaying partitions in \d+ output of a partitioned table
annotate the partitioned partitions as "has partitions".
For a partitioned table show the number of partitions even if it's 0.
Refactored an existing statement to use ? : instead of if condition
according to the surrounding code.
Ashutosh Bapat and Amit Langote.
---
src/bin/psql/describe.c| 41 +---
src/test/regress/expected/create_table.out | 13 +
src/test/regress/expected/foreign_data.out |3 ++
src/test/regress/expected/insert.out | 15 ++
src/test/regress/sql/create_table.sql |2 +-
src/test/regress/sql/insert.sql|4 +++
6 files changed, 62 insertions(+), 16 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f6049cc..db97de0 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2828,7 +2828,7 @@ describeOneTableDetails(const char *schemaname,
/* print child tables (with additional info if partitions) */
if (pset.sversion >= 10)
printfPQExpBuffer(&buf,
- "SELECT c.oid::pg_catalog.regclass, pg_catalog.pg_get_expr(c.relpartbound, c.oid)"
+ "SELECT c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid), c.relkind"
" FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i"
" WHERE c.oid=i.inhrelid AND i.inhparent = '%s'"
" ORDER BY c.oid::pg_catalog.regclass::pg_catalog.text;", oid);
@@ -2851,15 +2851,26 @@ describeOneTableDetails(const char *schemaname,
else
tuples = PQntuples(result);
- if (!verbose)
+ /*
+ * For a partitioned table with no partitions, always print the number
+ * of partitions as zero, even when verbose output is expected.
+ * Otherwise, we will not print "Partitions" section for a partitioned
+ * table without any partitions.
+ */
+ if (tableinfo.relkind == RELKIND_PARTITIONED_TABLE && tuples == 0)
+ {
+ printfPQExpBuffer(&buf, _("Number of partitions: %d"), tuples);
+ printTableAddFooter(&cont, buf.data);
+ }
+ else if (!verbose)
{
+ const char *ct = tableinfo.relkind != RELKIND_PARTITIONED_TABLE ?
+ _("child tables") : _("partitions");
+
/* print the number of child tables, if any */
if (tuples > 0)
{
-if (tableinfo.relkind != RELKIND_PARTITIONED_TABLE)
- printfPQExpBuffer(&buf, _("Number of child tables: %d (Use \\d+ to list them.)"), tuples);
-else
- printfPQExpBuffer(&buf, _("Number of partitions: %d (Use \\d+ to list them.)"), tuples);
+printfPQExpBuffer(&buf, _("Number of %s: %d (Use \\d+ to list them.)"), ct, tuples);
printTableAddFooter(&cont, buf.data);
}
}
@@ -2883,12 +2894,21 @@ describeOneTableDetails(const char *schemaname,
}
else
{
+ char *partitioned_note;
+
+ if (*PQgetvalue(result, i, 2) == RELKIND_PARTITIONED_TABLE)
+ partitioned_note = " has partitions";
+ else
+ partitioned_note = "";
+
if (i == 0)
- printfPQExpBuffer(&buf, "%s: %s
Re: [HACKERS] dropping partitioned tables without CASCADE
Added this to 2017/7 commitfest to keep a track of it. On Wed, Mar 8, 2017 at 3:39 PM, Amit Langote wrote: > On 2017/03/08 18:27, Ashutosh Bapat wrote: >>> >>> About the other statement you changed, I just realized that we should >>> perhaps do one more thing. Show the Number of partitions, even if it's 0. >>> In case of inheritance, the parent table stands on its own when there are >>> no child tables, but a partitioned table doesn't in the same sense. I >>> tried to implement that in attached patch 0002. Example below: >>> >>> create table p (a int) partition by list (a); >>> \d p >>> >>> Partition key: LIST (a) >>> Number of partitions: 0 >>> >>> \d+ p >>> >>> Partition key: LIST (a) >>> Number of partitions: 0 >>> >>> create table p1 partition of p for values in (1); >>> \d p >>> >>> Partition key: LIST (a) >>> Number of partitions: 1 (Use \d+ to list them.) >>> >>> \d+ p >>> >>> Partition key: LIST (a) >>> Partitions: p1 FOR VALUES IN (1) >> >> I liked that. PFA 0002 updated. I changed one of \d output to \d+ to >> better test partitioned tables without partitions in verbose and >> non-verbose mode. Also, refactored the your code to have less number >> of conditions. Please let me know if it looks good. > > Thanks, looks good. > > Regards, > Amit > > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/03/08 18:27, Ashutosh Bapat wrote: >> >> About the other statement you changed, I just realized that we should >> perhaps do one more thing. Show the Number of partitions, even if it's 0. >> In case of inheritance, the parent table stands on its own when there are >> no child tables, but a partitioned table doesn't in the same sense. I >> tried to implement that in attached patch 0002. Example below: >> >> create table p (a int) partition by list (a); >> \d p >> >> Partition key: LIST (a) >> Number of partitions: 0 >> >> \d+ p >> >> Partition key: LIST (a) >> Number of partitions: 0 >> >> create table p1 partition of p for values in (1); >> \d p >> >> Partition key: LIST (a) >> Number of partitions: 1 (Use \d+ to list them.) >> >> \d+ p >> >> Partition key: LIST (a) >> Partitions: p1 FOR VALUES IN (1) > > I liked that. PFA 0002 updated. I changed one of \d output to \d+ to > better test partitioned tables without partitions in verbose and > non-verbose mode. Also, refactored the your code to have less number > of conditions. Please let me know if it looks good. Thanks, looks good. Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
> > About the other statement you changed, I just realized that we should > perhaps do one more thing. Show the Number of partitions, even if it's 0. > In case of inheritance, the parent table stands on its own when there are > no child tables, but a partitioned table doesn't in the same sense. I > tried to implement that in attached patch 0002. Example below: > > create table p (a int) partition by list (a); > \d p > > Partition key: LIST (a) > Number of partitions: 0 > > \d+ p > > Partition key: LIST (a) > Number of partitions: 0 > > create table p1 partition of p for values in (1); > \d p > > Partition key: LIST (a) > Number of partitions: 1 (Use \d+ to list them.) > > \d+ p > > Partition key: LIST (a) > Partitions: p1 FOR VALUES IN (1) I liked that. PFA 0002 updated. I changed one of \d output to \d+ to better test partitioned tables without partitions in verbose and non-verbose mode. Also, refactored the your code to have less number of conditions. Please let me know if it looks good. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company 0001-Improve-d-output-of-a-partitioned-table.patch Description: Binary data 0002-Number-of-partitions-for-a-partitioned-table.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
Hi Ashutosh,
On 2017/03/06 18:19, Ashutosh Bapat wrote:
> On Mon, Mar 6, 2017 at 11:12 AM, Ashutosh Bapat wrote:
>> On Mon, Mar 6, 2017 at 11:05 AM, Simon Riggs wrote:
>>> "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0)
>>> TO (100)" is. So ISTM sensible to differentiate between DDL and
>>> non-ddl using upper and lower case.
>>>
>>
>> Make sense. Will try to cook up a patch soon.
>
> here's the patch. I have added a testcase in insert.sql to test \d+
> output for a partitioned table which has partitions which are further
> partitioned and also some partitions which are not partitioned
> themselves. I have also refactored a statement few lines above,
> replacing an if condition with ? : operator similar to code few lines
> below.
Thanks for cooking up the patch. Looks good and works as expected.
Regression tests pass.
About the other statement you changed, I just realized that we should
perhaps do one more thing. Show the Number of partitions, even if it's 0.
In case of inheritance, the parent table stands on its own when there are
no child tables, but a partitioned table doesn't in the same sense. I
tried to implement that in attached patch 0002. Example below:
create table p (a int) partition by list (a);
\d p
Partition key: LIST (a)
Number of partitions: 0
\d+ p
Partition key: LIST (a)
Number of partitions: 0
create table p1 partition of p for values in (1);
\d p
Partition key: LIST (a)
Number of partitions: 1 (Use \d+ to list them.)
\d+ p
Partition key: LIST (a)
Partitions: p1 FOR VALUES IN (1)
0001 is your original patch.
Thanks,
Amit
>From f972a1231e66b84de0b1922e6a6fd96ea661ae20 Mon Sep 17 00:00:00 2001
From: amit
Date: Wed, 8 Mar 2017 11:21:55 +0900
Subject: [PATCH 1/2] Ashutosh's patch to improve \dt+ of partitioned tables
---
src/bin/psql/describe.c | 26 +-
src/test/regress/expected/insert.out | 15 +++
src/test/regress/sql/insert.sql | 4
3 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e2e4cbcc08..f8617cf328 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2684,7 +2684,7 @@ describeOneTableDetails(const char *schemaname,
/* print child tables (with additional info if partitions) */
if (pset.sversion >= 10)
printfPQExpBuffer(&buf,
- "SELECT c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid)"
+ "SELECT c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid), c.relkind"
" FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i"
" WHERE c.oid=i.inhrelid AND"
" i.inhparent = '%s' AND"
@@ -2709,13 +2709,12 @@ describeOneTableDetails(const char *schemaname,
if (!verbose)
{
+ const char *ct = tableinfo.relkind != 'P' ? _("child tables") : _("partitions");
+
/* print the number of child tables, if any */
if (tuples > 0)
{
-if (tableinfo.relkind != 'P')
- printfPQExpBuffer(&buf, _("Number of child tables: %d (Use \\d+ to list them.)"), tuples);
-else
- printfPQExpBuffer(&buf, _("Number of partitions: %d (Use \\d+ to list them.)"), tuples);
+printfPQExpBuffer(&buf, _("Number of %s: %d (Use \\d+ to list them.)"), ct, tuples);
printTableAddFooter(&cont, buf.data);
}
}
@@ -2738,12 +2737,21 @@ describeOneTableDetails(const char *schemaname,
}
else
{
+ char *partitioned_note;
+
+ if (*PQgetvalue(result, i, 2) == 'P')
+ partitioned_note = " has partitions";
+ else
+ partitioned_note = "";
+
if (i == 0)
- printfPQExpBuffer(&buf, "%s: %s %s",
- ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%s: %s %s%s",
+ ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+ partitioned_note);
else
- printfPQExpBuffer(&buf, "%*s %s %s",
- ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+ printfPQExpBuffer(&buf, "%*s %s %s%s",
+ ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+ partitioned_note);
}
if (i < tuples - 1)
appendPQExpBufferChar(&buf, ',');
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 116854e142..c8ff863882 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -313,6 +313,21 @@ select tableoid::regclass::text, a, min(b) as min_b, max(b) as max_b from list_p
part_null || 1 | 1
(9 rows)
+-- test \d+ output on a table which has both partitioned and unpartitioned
+-- partitions
+\d+ list_parted
+Table "public.list_parted"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
++-+---+--+-+--+--+-
+ a | text| | | | ext
Re: [HACKERS] dropping partitioned tables without CASCADE
On Mon, Mar 6, 2017 at 11:12 AM, Ashutosh Bapat wrote: > On Mon, Mar 6, 2017 at 11:05 AM, Simon Riggs wrote: >> On 6 March 2017 at 05:29, Ashutosh Bapat >> wrote: >> >>> Just to confirm, you want the output to look like this > \d+ t1 > Table "public.t1" > Column | Type | Collation | Nullable | Default | Storage | Stats > target | Description > +-+---+--+-+-+--+- > a | integer | | not null | | plain | > | > Partition key: RANGE (a) > Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS > t1p2 FOR VALUES FROM (100) TO (200) >>> lowercase please >>> >>> Except for HAS PARTITIONS, everything is part of today's output. Given >>> the current output, HAS PARTITIONS should be in upper case. >> >> "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0) >> TO (100)" is. So ISTM sensible to differentiate between DDL and >> non-ddl using upper and lower case. >> > > Make sense. Will try to cook up a patch soon. here's the patch. I have added a testcase in insert.sql to test \d+ output for a partitioned table which has partitions which are further partitioned and also some partitions which are not partitioned themselves. I have also refactored a statement few lines above, replacing an if condition with ? : operator similar to code few lines below. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company pg_part_desc.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/03/06 14:25, Simon Riggs wrote: > On 6 March 2017 at 04:00, Ashutosh Bapat wrote: >> Thinking about how to display partition which are further partitioned, >> there are two options. Assume a partitioned table t1 with partitions >> t1p1, which is further partitioned and t1p2. One could display \d+ t1 >> as >> >> \d+ t1 >> Table "public.t1" >> Column | Type | Collation | Nullable | Default | Storage | Stats >> target | Description >> +-+---+--+-+-+--+- >> a | integer | | not null | | plain | | >> Partition key: RANGE (a) >> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS >> t1p2 FOR VALUES FROM (100) TO (200) >> >> OR >> >> \d+ t1 >> Table "public.t1" >> Column | Type | Collation | Nullable | Default | Storage | Stats >> target | Description >> +-+---+--+-+-+--+- >> a | integer | | not null | | plain | | >> Partition key: RANGE (a) >> Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a) >> t1p2 FOR VALUES FROM (100) TO (200) >> >> To me the first option looks fine. > > +1 > > lowercase please +1 Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On Mon, Mar 6, 2017 at 11:05 AM, Simon Riggs wrote: > On 6 March 2017 at 05:29, Ashutosh Bapat > wrote: > >> Just to confirm, you want the output to look like this \d+ t1 Table "public.t1" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- a | integer | | not null | | plain | | Partition key: RANGE (a) Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS t1p2 FOR VALUES FROM (100) TO (200) >> >>> >>> lowercase please >> >> Except for HAS PARTITIONS, everything is part of today's output. Given >> the current output, HAS PARTITIONS should be in upper case. > > "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0) > TO (100)" is. So ISTM sensible to differentiate between DDL and > non-ddl using upper and lower case. > Make sense. Will try to cook up a patch soon. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 6 March 2017 at 05:29, Ashutosh Bapat wrote: > Just to confirm, you want the output to look like this >>> \d+ t1 >>> Table "public.t1" >>> Column | Type | Collation | Nullable | Default | Storage | Stats >>> target | Description >>> +-+---+--+-+-+--+- >>> a | integer | | not null | | plain | >>> | >>> Partition key: RANGE (a) >>> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS >>> t1p2 FOR VALUES FROM (100) TO (200) > >> >> lowercase please > > Except for HAS PARTITIONS, everything is part of today's output. Given > the current output, HAS PARTITIONS should be in upper case. "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0) TO (100)" is. So ISTM sensible to differentiate between DDL and non-ddl using upper and lower case. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On Mon, Mar 6, 2017 at 10:55 AM, Simon Riggs wrote: > On 6 March 2017 at 04:00, Ashutosh Bapat > wrote: >> On Mon, Mar 6, 2017 at 8:35 AM, Simon Riggs wrote: >>> On 6 March 2017 at 00:51, Amit Langote >>> wrote: On 2017/03/05 16:20, Simon Riggs wrote: > I notice also that > \d+ > does not show which partitions have subpartitions. Do you mean showing just whether a partition is itself partitioned or showing its partitions and so on (because those partitions may themselves be partitioned)? Maybe, we could do the former. >>> >>> I think \d+ should show the full information, in some form. >> >> For a multi-level inheritance hierarchy, we don't show children which >> are further inherited. Same behaviour has been carried over to >> partitioning. I don't say that that's good or bad. >> >> Given the recursive structure of partitioned tables, it looks readable >> and manageable to print only the direct partitions in \d+. May be we >> want to indicate the partitions that are further partitioned. If user >> wants information about partitioned partitions, s/he can execute \d+ >> on the partition, repeating this process to any desired level. This >> would work well in the interactive mode, keeping the output of \d+ >> manageable. Further someone writing a script to consume \d+ output of >> a multi-level partitioned table, can code the above process in a >> script. >> >> Thinking about how to display partition which are further partitioned, >> there are two options. Assume a partitioned table t1 with partitions >> t1p1, which is further partitioned and t1p2. One could display \d+ t1 >> as >> >> \d+ t1 >> Table "public.t1" >> Column | Type | Collation | Nullable | Default | Storage | Stats >> target | Description >> +-+---+--+-+-+--+- >> a | integer | | not null | | plain | | >> Partition key: RANGE (a) >> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS >> t1p2 FOR VALUES FROM (100) TO (200) >> >> OR >> >> \d+ t1 >> Table "public.t1" >> Column | Type | Collation | Nullable | Default | Storage | Stats >> target | Description >> +-+---+--+-+-+--+- >> a | integer | | not null | | plain | | >> Partition key: RANGE (a) >> Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a) >> t1p2 FOR VALUES FROM (100) TO (200) >> >> To me the first option looks fine. > > +1 Just to confirm, you want the output to look like this >> \d+ t1 >> Table "public.t1" >> Column | Type | Collation | Nullable | Default | Storage | Stats >> target | Description >> +-+---+--+-+-+--+- >> a | integer | | not null | | plain | | >> Partition key: RANGE (a) >> Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS >> t1p2 FOR VALUES FROM (100) TO (200) > > lowercase please Except for HAS PARTITIONS, everything is part of today's output. Given the current output, HAS PARTITIONS should be in upper case. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 6 March 2017 at 04:00, Ashutosh Bapat wrote: > On Mon, Mar 6, 2017 at 8:35 AM, Simon Riggs wrote: >> On 6 March 2017 at 00:51, Amit Langote wrote: >>> On 2017/03/05 16:20, Simon Riggs wrote: I notice also that \d+ does not show which partitions have subpartitions. >>> >>> Do you mean showing just whether a partition is itself partitioned or >>> showing its partitions and so on (because those partitions may themselves >>> be partitioned)? Maybe, we could do the former. >> >> I think \d+ should show the full information, in some form. > > For a multi-level inheritance hierarchy, we don't show children which > are further inherited. Same behaviour has been carried over to > partitioning. I don't say that that's good or bad. > > Given the recursive structure of partitioned tables, it looks readable > and manageable to print only the direct partitions in \d+. May be we > want to indicate the partitions that are further partitioned. If user > wants information about partitioned partitions, s/he can execute \d+ > on the partition, repeating this process to any desired level. This > would work well in the interactive mode, keeping the output of \d+ > manageable. Further someone writing a script to consume \d+ output of > a multi-level partitioned table, can code the above process in a > script. > > Thinking about how to display partition which are further partitioned, > there are two options. Assume a partitioned table t1 with partitions > t1p1, which is further partitioned and t1p2. One could display \d+ t1 > as > > \d+ t1 > Table "public.t1" > Column | Type | Collation | Nullable | Default | Storage | Stats > target | Description > +-+---+--+-+-+--+- > a | integer | | not null | | plain | | > Partition key: RANGE (a) > Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS > t1p2 FOR VALUES FROM (100) TO (200) > > OR > > \d+ t1 > Table "public.t1" > Column | Type | Collation | Nullable | Default | Storage | Stats > target | Description > +-+---+--+-+-+--+- > a | integer | | not null | | plain | | > Partition key: RANGE (a) > Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a) > t1p2 FOR VALUES FROM (100) TO (200) > > To me the first option looks fine. +1 lowercase please -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On Mon, Mar 6, 2017 at 8:35 AM, Simon Riggs wrote: > On 6 March 2017 at 00:51, Amit Langote wrote: >> On 2017/03/05 16:20, Simon Riggs wrote: >>> I notice also that >>> \d+ >>> does not show which partitions have subpartitions. >> >> Do you mean showing just whether a partition is itself partitioned or >> showing its partitions and so on (because those partitions may themselves >> be partitioned)? Maybe, we could do the former. > > I think \d+ should show the full information, in some form. For a multi-level inheritance hierarchy, we don't show children which are further inherited. Same behaviour has been carried over to partitioning. I don't say that that's good or bad. Given the recursive structure of partitioned tables, it looks readable and manageable to print only the direct partitions in \d+. May be we want to indicate the partitions that are further partitioned. If user wants information about partitioned partitions, s/he can execute \d+ on the partition, repeating this process to any desired level. This would work well in the interactive mode, keeping the output of \d+ manageable. Further someone writing a script to consume \d+ output of a multi-level partitioned table, can code the above process in a script. Thinking about how to display partition which are further partitioned, there are two options. Assume a partitioned table t1 with partitions t1p1, which is further partitioned and t1p2. One could display \d+ t1 as \d+ t1 Table "public.t1" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- a | integer | | not null | | plain | | Partition key: RANGE (a) Partitions: t1p1 FOR VALUES FROM (0) TO (100), HAS PARTITIONS t1p2 FOR VALUES FROM (100) TO (200) OR \d+ t1 Table "public.t1" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- a | integer | | not null | | plain | | Partition key: RANGE (a) Partitions: t1p1 FOR VALUES FROM (0) TO (100), PARTITION BY LIST(a) t1p2 FOR VALUES FROM (100) TO (200) To me the first option looks fine. If the user is interested in looking at the subpartitioned in any case s/he will have to execute \d+. So beyond indicating that a partition has subpartitions, what other information to include is debatable, given that \d+ on that partition is going to print it anyway. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 6 March 2017 at 00:51, Amit Langote wrote: > On 2017/03/05 16:20, Simon Riggs wrote: >> I notice also that >> \d+ >> does not show which partitions have subpartitions. > > Do you mean showing just whether a partition is itself partitioned or > showing its partitions and so on (because those partitions may themselves > be partitioned)? Maybe, we could do the former. I think \d+ should show the full information, in some form. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/03/05 16:20, Simon Riggs wrote: > I notice also that > \d+ > does not show which partitions have subpartitions. Do you mean showing just whether a partition is itself partitioned or showing its partitions and so on (because those partitions may themselves be partitioned)? Maybe, we could do the former. > I'm worried that these things illustrate something about the catalog > representation that we may need to improve, but I don't have anything > concrete to say on that at present. Perhaps. As Ashutosh said though, it does not seem like a big problem in this particular case. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 5 March 2017 at 07:59, Ashutosh Bapat
wrote:
>>
>> I used a slight modification of the case mentioned on the docs. I
>> confirm this fails repeatably for me on current HEAD.
>>
>> CREATE TABLE cities (
>> city_id bigserial not null,
>> name text not null,
>> population bigint
>> ) PARTITION BY LIST (left(lower(name), 1));
>>
>> CREATE TABLE cities_ab
>> PARTITION OF cities (
>> CONSTRAINT city_id_nonzero CHECK (city_id != 0)
>> ) FOR VALUES IN ('a', 'b')
>> PARTITION BY RANGE (population);
>>
>> drop table cities;
>> ERROR: cannot drop table cities because other objects depend on it
>> DETAIL: table cities_ab depends on table cities
>> HINT: Use DROP ... CASCADE to drop the dependent objects too.
>
> I think that's what this patch fixes. Do you see this behaviour after
> applying the patch?
It does seems as if I've made a mistake there. The patch passes.
Thanks for checking.
I will apply tomorrow if no further comments.
--
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
>
> I used a slight modification of the case mentioned on the docs. I
> confirm this fails repeatably for me on current HEAD.
>
> CREATE TABLE cities (
> city_id bigserial not null,
> name text not null,
> population bigint
> ) PARTITION BY LIST (left(lower(name), 1));
>
> CREATE TABLE cities_ab
> PARTITION OF cities (
> CONSTRAINT city_id_nonzero CHECK (city_id != 0)
> ) FOR VALUES IN ('a', 'b')
> PARTITION BY RANGE (population);
>
> drop table cities;
> ERROR: cannot drop table cities because other objects depend on it
> DETAIL: table cities_ab depends on table cities
> HINT: Use DROP ... CASCADE to drop the dependent objects too.
I think that's what this patch fixes. Do you see this behaviour after
applying the patch?
>
> I notice also that
> \d+
> does not show which partitions have subpartitions.
I will confirm this once I have access to the code.
>
> I'm worried that these things illustrate something about the catalog
> representation that we may need to improve, but I don't have anything
> concrete to say on that at present.
AFAIK, catalogs have everything needed to fix this; it's just the
matter to using that information wherever it's needed.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 27 February 2017 at 02:38, Amit Langote
wrote:
> On 2017/02/26 5:30, Simon Riggs wrote:
>> On 23 February 2017 at 16:33, Simon Riggs wrote:
>>
>>> I'll be happy to review
>>
>> Patch looks OK so far, but fails on a partition that has partitions,
>> probably because of the way we test relkind in the call to
>> StoreCatalogInheritance1().
>
> Thanks for the review.
>
> I could not reproduce the failure you are seeing; could you perhaps share
> the failing test case?
I used a slight modification of the case mentioned on the docs. I
confirm this fails repeatably for me on current HEAD.
CREATE TABLE cities (
city_id bigserial not null,
name text not null,
population bigint
) PARTITION BY LIST (left(lower(name), 1));
CREATE TABLE cities_ab
PARTITION OF cities (
CONSTRAINT city_id_nonzero CHECK (city_id != 0)
) FOR VALUES IN ('a', 'b')
PARTITION BY RANGE (population);
drop table cities;
ERROR: cannot drop table cities because other objects depend on it
DETAIL: table cities_ab depends on table cities
HINT: Use DROP ... CASCADE to drop the dependent objects too.
I notice also that
\d+
does not show which partitions have subpartitions.
I'm worried that these things illustrate something about the catalog
representation that we may need to improve, but I don't have anything
concrete to say on that at present.
--
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
>> >> Isn't list_range_parted multilevel partitioned table. It gets dropped >> in the testcases. So, I guess, we already have a testcase there. > > I thought Simon meant the test case where a partition that is itself > partitioned is dropped. At least that's what I took from "... fails *on* > partition that has partitions". So in the example I posted, drop table p1. Ok. Thanks for the explanation. > > Anyway, there might be the confusion that *only* the root level > partitioned table is of RELKIND_PARTITIONED_TABLE. That's not true - any > partitioned table (even one that's a partition) is of that relkind. So > the condition in the call to StoreCatalogInheritance1() is correct. The > following hunk: > > @@ -10744,7 +10756,9 @@ CreateInheritance(Relation child_rel, Relation > parent_rel) > StoreCatalogInheritance1(RelationGetRelid(child_rel), > RelationGetRelid(parent_rel), > inhseqno + 1, > - catalogRelation); > + catalogRelation, > + parent_rel->rd_rel->relkind == > +RELKIND_PARTITIONED_TABLE); > > Thanks, > Amit > > I agree. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/27 13:35, Ashutosh Bapat wrote:
> On Mon, Feb 27, 2017 at 8:08 AM, Amit Langote
> wrote:
>> On 2017/02/26 5:30, Simon Riggs wrote:
>>> On 23 February 2017 at 16:33, Simon Riggs wrote:
>>>
I'll be happy to review
>>>
>>> Patch looks OK so far, but fails on a partition that has partitions,
>>> probably because of the way we test relkind in the call to
>>> StoreCatalogInheritance1().
>>
>> Thanks for the review.
>>
>> I could not reproduce the failure you are seeing; could you perhaps share
>> the failing test case? Here's mine that seems to work as expected:
>>
>> create table p (a int, b char) partition by list (a);
>> create table p1 (a int, b char) partition by list (b);
>> alter table p attach partition p1 for values in (1);
>>
>> -- add a partition to p1
>> create table p1a (like p1);
>> alter table p1 attach partition p1a for values in ('a');
>>
>> create table p2 partition of p for values in (1)
>>
>> \d+ p
>>
>> Partition key: LIST (a)
>> Partitions: p1 FOR VALUES IN (1),
>> p2 FOR VALUES IN (2)
>>
>> -- this works (remember that p1 is a partitioned table)
>> drop table p1;
>> DROP TABLE
>>
>> \d+ p
>>
>> Partition key: LIST (a)
>> Partitions: p2 FOR VALUES IN (2)
>>
>>> Please add a test for that so we can check automatically.
>>
>> OK, done.
>
> Isn't list_range_parted multilevel partitioned table. It gets dropped
> in the testcases. So, I guess, we already have a testcase there.
I thought Simon meant the test case where a partition that is itself
partitioned is dropped. At least that's what I took from "... fails *on*
partition that has partitions". So in the example I posted, drop table p1.
Anyway, there might be the confusion that *only* the root level
partitioned table is of RELKIND_PARTITIONED_TABLE. That's not true - any
partitioned table (even one that's a partition) is of that relkind. So
the condition in the call to StoreCatalogInheritance1() is correct. The
following hunk:
@@ -10744,7 +10756,9 @@ CreateInheritance(Relation child_rel, Relation
parent_rel)
StoreCatalogInheritance1(RelationGetRelid(child_rel),
RelationGetRelid(parent_rel),
inhseqno + 1,
- catalogRelation);
+ catalogRelation,
+ parent_rel->rd_rel->relkind ==
+RELKIND_PARTITIONED_TABLE);
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On Mon, Feb 27, 2017 at 8:08 AM, Amit Langote
wrote:
> On 2017/02/26 5:30, Simon Riggs wrote:
>> On 23 February 2017 at 16:33, Simon Riggs wrote:
>>
>>> I'll be happy to review
>>
>> Patch looks OK so far, but fails on a partition that has partitions,
>> probably because of the way we test relkind in the call to
>> StoreCatalogInheritance1().
>
> Thanks for the review.
>
> I could not reproduce the failure you are seeing; could you perhaps share
> the failing test case? Here's mine that seems to work as expected:
>
> create table p (a int, b char) partition by list (a);
> create table p1 (a int, b char) partition by list (b);
> alter table p attach partition p1 for values in (1);
>
> -- add a partition to p1
> create table p1a (like p1);
> alter table p1 attach partition p1a for values in ('a');
>
> create table p2 partition of p for values in (1)
>
> \d+ p
>
> Partition key: LIST (a)
> Partitions: p1 FOR VALUES IN (1),
> p2 FOR VALUES IN (2)
>
> -- this works (remember that p1 is a partitioned table)
> drop table p1;
> DROP TABLE
>
> \d+ p
>
> Partition key: LIST (a)
> Partitions: p2 FOR VALUES IN (2)
>
>> Please add a test for that so we can check automatically.
>
> OK, done.
Isn't list_range_parted multilevel partitioned table. It gets dropped
in the testcases. So, I guess, we already have a testcase there.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/26 5:30, Simon Riggs wrote:
> On 23 February 2017 at 16:33, Simon Riggs wrote:
>
>> I'll be happy to review
>
> Patch looks OK so far, but fails on a partition that has partitions,
> probably because of the way we test relkind in the call to
> StoreCatalogInheritance1().
Thanks for the review.
I could not reproduce the failure you are seeing; could you perhaps share
the failing test case? Here's mine that seems to work as expected:
create table p (a int, b char) partition by list (a);
create table p1 (a int, b char) partition by list (b);
alter table p attach partition p1 for values in (1);
-- add a partition to p1
create table p1a (like p1);
alter table p1 attach partition p1a for values in ('a');
create table p2 partition of p for values in (1)
\d+ p
Partition key: LIST (a)
Partitions: p1 FOR VALUES IN (1),
p2 FOR VALUES IN (2)
-- this works (remember that p1 is a partitioned table)
drop table p1;
DROP TABLE
\d+ p
Partition key: LIST (a)
Partitions: p2 FOR VALUES IN (2)
> Please add a test for that so we can check automatically.
OK, done.
Thanks,
Amit
>From 419768af093d1d7b4bd9f57e2c9481227499aa1c Mon Sep 17 00:00:00 2001
From: amit
Date: Thu, 16 Feb 2017 15:56:44 +0900
Subject: [PATCH] Allow dropping partitioned table without CASCADE
Currently, a normal dependency is created between a inheritance
parent and child when creating the child. That means one must
specify CASCADE to drop the parent table if a child table exists.
When creating partitions as inheritance children, create auto
dependency instead, so that partitions get dropped automatically
when the parent is dropped i.e., without specifying CASCADE.
---
src/backend/commands/tablecmds.c | 30 ---
src/test/regress/expected/alter_table.out | 10
src/test/regress/expected/create_table.out | 38 --
src/test/regress/expected/inherit.out | 22 ++---
src/test/regress/expected/insert.out | 7 ++
src/test/regress/expected/update.out | 7 +-
src/test/regress/sql/alter_table.sql | 10
src/test/regress/sql/create_table.sql | 23 --
src/test/regress/sql/inherit.sql | 4 ++--
src/test/regress/sql/insert.sql| 7 ++
src/test/regress/sql/update.sql| 2 +-
11 files changed, 87 insertions(+), 73 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3cea220421..3753d66c9e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -289,9 +289,11 @@ static List *MergeAttributes(List *schema, List *supers, char relpersistence,
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
-static void StoreCatalogInheritance(Oid relationId, List *supers);
+static void StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation);
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition);
static int findAttrByName(const char *attributeName, List *schema);
static void AlterIndexNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
@@ -725,7 +727,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
typaddress);
/* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids);
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
/*
* We must bump the command counter to make the newly-created relation
@@ -2240,7 +2242,8 @@ MergeCheckConstraint(List *constraints, char *name, Node *expr)
* supers is a list of the OIDs of the new relation's direct ancestors.
*/
static void
-StoreCatalogInheritance(Oid relationId, List *supers)
+StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition)
{
Relation relation;
int16 seqNumber;
@@ -2270,7 +2273,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
{
Oid parentOid = lfirst_oid(entry);
- StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation);
+ StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation,
+ child_is_partition);
seqNumber++;
}
@@ -2283,7 +2287,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
*/
static void
StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation)
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition)
{
TupleDesc desc = RelationGetDescr(inhRelation);
Datum values[Natts_pg_inherits];
@@ -2317,7 +2322,14 @@ StoreCatalogInheritance1(Oid rela
Re: [HACKERS] dropping partitioned tables without CASCADE
On 23 February 2017 at 16:33, Simon Riggs wrote: > I'll be happy to review Patch looks OK so far, but fails on a partition that has partitions, probably because of the way we test relkind in the call to StoreCatalogInheritance1(). Please add a test for that so we can check automatically. Thanks very much. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/24 1:33, Simon Riggs wrote: > On 23 February 2017 at 06:40, Ashutosh Bapat > wrote: > >> I think this is ready for committer. > > Thanks for writing and reviewing this. I'll be happy to review and > commit. Please add to CF. Thanks. I've added it to CF: https://commitfest.postgresql.org/13/1030/ Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 23 February 2017 at 06:40, Ashutosh Bapat wrote: > I think this is ready for committer. Thanks for writing and reviewing this. I'll be happy to review and commit. Please add to CF. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
I think this is ready for committer. On Thu, Feb 23, 2017 at 12:02 PM, Amit Langote wrote: > On 2017/02/22 21:24, Ashutosh Bapat wrote: >> On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote wrote: >>> + /* >>> +* Unlike inheritance children, partition tables are expected to be >>> dropped >>> +* when the parent partitioned table gets dropped. >>> +*/ >>> >>> Hmm. Partitions *are* inheritance children, so we perhaps don't need the >>> part before the comma. Also, adding "automatically" somewhere in there >>> would be nice. >>> >>> Or, one could just write: /* add an auto dependency for partitions */ >> >> I changed it in the attached patch to >> +/* >> + * Partition tables are expected to be dropped when the parent >> partitioned >> + * table gets dropped. >> + */ > > OK, thanks. > > Thanks, > Amit > > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/22 21:24, Ashutosh Bapat wrote: > On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote wrote: >> + /* >> +* Unlike inheritance children, partition tables are expected to be >> dropped >> +* when the parent partitioned table gets dropped. >> +*/ >> >> Hmm. Partitions *are* inheritance children, so we perhaps don't need the >> part before the comma. Also, adding "automatically" somewhere in there >> would be nice. >> >> Or, one could just write: /* add an auto dependency for partitions */ > > I changed it in the attached patch to > +/* > + * Partition tables are expected to be dropped when the parent > partitioned > + * table gets dropped. > + */ OK, thanks. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On Wed, Feb 22, 2017 at 11:11 AM, Amit Langote wrote: > On 2017/02/22 13:46, Ashutosh Bapat wrote: >> Looks good to me. In the attached patch I have added a comment >> explaining the reason to make partition tables "Auto" dependent upon >> the corresponding partitioned tables. > > Good call. > > + /* > +* Unlike inheritance children, partition tables are expected to be > dropped > +* when the parent partitioned table gets dropped. > +*/ > > Hmm. Partitions *are* inheritance children, so we perhaps don't need the > part before the comma. Also, adding "automatically" somewhere in there > would be nice. > > Or, one could just write: /* add an auto dependency for partitions */ I changed it in the attached patch to +/* + * Partition tables are expected to be dropped when the parent partitioned + * table gets dropped. + */ > >> In the tests we are firing commands to drop partitioned table, but are >> not checking whether those tables or the partitions are getting >> dropped or not. Except for drop_if_exists.sql, I did not find that we >> really check this. Should we try a query on pg_class to ensure that >> the tables get really dropped? > > I don't see why this patch should do it, if dependency.sql itself does > not? I mean dropping AUTO dependent objects is one of the contracts of > dependency.c, so perhaps it would make sense to query pg_class in > dependency.sql to check if AUTO dependencies work correctly. Hmm, I agree. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company 0001-Allow-dropping-partitioned-table-without-CASCADE_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/22 13:46, Ashutosh Bapat wrote:
> Looks good to me. In the attached patch I have added a comment
> explaining the reason to make partition tables "Auto" dependent upon
> the corresponding partitioned tables.
Good call.
+ /*
+* Unlike inheritance children, partition tables are expected to be
dropped
+* when the parent partitioned table gets dropped.
+*/
Hmm. Partitions *are* inheritance children, so we perhaps don't need the
part before the comma. Also, adding "automatically" somewhere in there
would be nice.
Or, one could just write: /* add an auto dependency for partitions */
> In the tests we are firing commands to drop partitioned table, but are
> not checking whether those tables or the partitions are getting
> dropped or not. Except for drop_if_exists.sql, I did not find that we
> really check this. Should we try a query on pg_class to ensure that
> the tables get really dropped?
I don't see why this patch should do it, if dependency.sql itself does
not? I mean dropping AUTO dependent objects is one of the contracts of
dependency.c, so perhaps it would make sense to query pg_class in
dependency.sql to check if AUTO dependencies work correctly.
Thanks,
Amit
>From 682624f4562087bb05b2ff9f282080bcfcfb5233 Mon Sep 17 00:00:00 2001
From: amit
Date: Thu, 16 Feb 2017 15:56:44 +0900
Subject: [PATCH] Allow dropping partitioned table without CASCADE
Currently, a normal dependency is created between a inheritance
parent and child when creating the child. That means one must
specify CASCADE to drop the parent table if a child table exists.
When creating partitions as inheritance children, create auto
dependency instead, so that partitions are dropped automatically
when the parent is dropped i.e., without specifying CASCADE.
---
src/backend/commands/tablecmds.c | 26 ++
src/test/regress/expected/alter_table.out | 10 --
src/test/regress/expected/create_table.out | 9 ++---
src/test/regress/expected/inherit.out | 22 ++
src/test/regress/expected/insert.out | 7 ++-
src/test/regress/expected/update.out | 7 +--
src/test/regress/sql/alter_table.sql | 10 --
src/test/regress/sql/create_table.sql | 9 ++---
src/test/regress/sql/inherit.sql | 4 ++--
src/test/regress/sql/insert.sql| 7 ++-
src/test/regress/sql/update.sql| 2 +-
11 files changed, 40 insertions(+), 73 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3cea220421..cf566f974b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -289,9 +289,11 @@ static List *MergeAttributes(List *schema, List *supers, char relpersistence,
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
-static void StoreCatalogInheritance(Oid relationId, List *supers);
+static void StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation);
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition);
static int findAttrByName(const char *attributeName, List *schema);
static void AlterIndexNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
@@ -725,7 +727,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
typaddress);
/* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids);
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
/*
* We must bump the command counter to make the newly-created relation
@@ -2240,7 +2242,8 @@ MergeCheckConstraint(List *constraints, char *name, Node *expr)
* supers is a list of the OIDs of the new relation's direct ancestors.
*/
static void
-StoreCatalogInheritance(Oid relationId, List *supers)
+StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition)
{
Relation relation;
int16 seqNumber;
@@ -2270,7 +2273,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
{
Oid parentOid = lfirst_oid(entry);
- StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation);
+ StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation,
+ child_is_partition);
seqNumber++;
}
@@ -2283,7 +2287,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
*/
static void
StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation)
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition)
{
TupleDesc desc = RelationGetDescr(
Re: [HACKERS] dropping partitioned tables without CASCADE
Looks good to me. In the attached patch I have added a comment explaining the reason to make partition tables "Auto" dependent upon the corresponding partitioned tables. In the tests we are firing commands to drop partitioned table, but are not checking whether those tables or the partitions are getting dropped or not. Except for drop_if_exists.sql, I did not find that we really check this. Should we try a query on pg_class to ensure that the tables get really dropped? On Wed, Feb 22, 2017 at 7:26 AM, Amit Langote wrote: > On 2017/02/22 10:49, Amit Langote wrote: >> On 2017/02/21 20:17, Ashutosh Bapat wrote: >>> Are you sure you have attached the right patch? >> >> Oops, really fixed this time. > > Sorry again, 3rd time's a charm. I copy-paste the hunk below from the > patch file before I attach it to make sure: > > - recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); > + if (child_is_partition) > + recordDependencyOn(&childobject, &parentobject, > DEPENDENCY_AUTO); > + else > + recordDependencyOn(&childobject, &parentobject, > DEPENDENCY_NORMAL); > > Thanks, > Amit -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company 0001-Allow-dropping-partitioned-table-without-CASCADE_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/22 10:49, Amit Langote wrote:
> On 2017/02/21 20:17, Ashutosh Bapat wrote:
>> Are you sure you have attached the right patch?
>
> Oops, really fixed this time.
Sorry again, 3rd time's a charm. I copy-paste the hunk below from the
patch file before I attach it to make sure:
- recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+ if (child_is_partition)
+ recordDependencyOn(&childobject, &parentobject,
DEPENDENCY_AUTO);
+ else
+ recordDependencyOn(&childobject, &parentobject,
DEPENDENCY_NORMAL);
Thanks,
Amit
>From 682624f4562087bb05b2ff9f282080bcfcfb5233 Mon Sep 17 00:00:00 2001
From: amit
Date: Thu, 16 Feb 2017 15:56:44 +0900
Subject: [PATCH] Allow dropping partitioned table without CASCADE
Currently, a normal dependency is created between a inheritance
parent and child when creating the child. That means one must
specify CASCADE to drop the parent table if a child table exists.
When creating partitions as inheritance children, create auto
dependency instead, so that partitions are dropped automatically
when the parent is dropped i.e., without specifying CASCADE.
---
src/backend/commands/tablecmds.c | 26 ++
src/test/regress/expected/alter_table.out | 10 --
src/test/regress/expected/create_table.out | 9 ++---
src/test/regress/expected/inherit.out | 22 ++
src/test/regress/expected/insert.out | 7 ++-
src/test/regress/expected/update.out | 7 +--
src/test/regress/sql/alter_table.sql | 10 --
src/test/regress/sql/create_table.sql | 9 ++---
src/test/regress/sql/inherit.sql | 4 ++--
src/test/regress/sql/insert.sql| 7 ++-
src/test/regress/sql/update.sql| 2 +-
11 files changed, 40 insertions(+), 73 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3cea220421..cf566f974b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -289,9 +289,11 @@ static List *MergeAttributes(List *schema, List *supers, char relpersistence,
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
-static void StoreCatalogInheritance(Oid relationId, List *supers);
+static void StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation);
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition);
static int findAttrByName(const char *attributeName, List *schema);
static void AlterIndexNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
@@ -725,7 +727,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
typaddress);
/* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids);
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
/*
* We must bump the command counter to make the newly-created relation
@@ -2240,7 +2242,8 @@ MergeCheckConstraint(List *constraints, char *name, Node *expr)
* supers is a list of the OIDs of the new relation's direct ancestors.
*/
static void
-StoreCatalogInheritance(Oid relationId, List *supers)
+StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition)
{
Relation relation;
int16 seqNumber;
@@ -2270,7 +2273,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
{
Oid parentOid = lfirst_oid(entry);
- StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation);
+ StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation,
+ child_is_partition);
seqNumber++;
}
@@ -2283,7 +2287,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
*/
static void
StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation)
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition)
{
TupleDesc desc = RelationGetDescr(inhRelation);
Datum values[Natts_pg_inherits];
@@ -2317,7 +2322,10 @@ StoreCatalogInheritance1(Oid relationId, Oid parentOid,
childobject.objectId = relationId;
childobject.objectSubId = 0;
- recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+ if (child_is_partition)
+ recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
+ else
+ recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
/*
* Post creation hook of this inheritance. Since object_access_hook
@@ -10744,7 +10752,9 @@ CreateInheritance(Relation child_rel, Relation parent_rel)
StoreCatalogInheritance1(RelationGet
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/21 20:17, Ashutosh Bapat wrote: > On Tue, Feb 21, 2017 at 12:05 PM, Amit Langote wrote: >> On 2017/02/20 21:49, Ashutosh Bapat wrote: >>> Here are some comments >>> >>> For the sake of readability you may want reverse the if and else order. >>> -recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); >>> +if (!child_is_partition) >>> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); >>> +else >>> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); >>> like >>> +if (child_is_partition) >>> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); >>> +else >>> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); >> >> Sure, done. > > I still see > -recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); > +if (!child_is_partition) > +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); > +else > +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); > > Are you sure you have attached the right patch? Oops, really fixed this time. >>> --- cleanup: avoid using CASCADE >>> -DROP TABLE list_parted, part_1; >>> -DROP TABLE list_parted2, part_2, part_5, part_5_a; >>> -DROP TABLE range_parted, part1, part2; >>> +-- cleanup >>> +DROP TABLE list_parted, list_parted2, range_parted; >>> Testcases usually drop one table at a time, I guess, to reduce the >>> differences >>> when we add or remove tables from testcases. All such blocks should probably >>> follow same policy. >> >> Hmm, I see this in src/test/regress/sql/inherit.sql:141 >> >> DROP TABLE firstparent, secondparent, jointchild, thirdparent, otherchild; > > Hmm, I can spot some more such usages. Let's keep this for the > committer to decide. Sure. >>> BTW, I noticed that although we are allowing foreign tables to be >>> partitions, there are no tests in foreign_data.sql for testing it. If >>> there would have been we would tests DROP TABLE on a partitioned table >>> with foreign partitions as well. That file has testcases for testing >>> foreign table inheritance, and should have tests for foreign table >>> partitions. >> >> That makes sense. Patch 0002 is for that (I'm afraid this should be >> posted separately though). I didn't add/repeat all the tests that were >> added by the foreign table inheritance patch again for foreign partitions >> (common inheritance rules apply to both cases), only added those for the >> new partitioning commands and certain new rules. > > Thanks. Yes, a separate thread would do. I will review it there. May > be you want to add it to the commitfest too. Posted in a new thread titled "foreign partition DDL regression tests". Thanks, Amit >From 525da2525b69e5191abac68f074d1d36c78bef8c Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 16 Feb 2017 15:56:44 +0900 Subject: [PATCH] Allow dropping partitioned table without CASCADE Currently, a normal dependency is created between a inheritance parent and child when creating the child. That means one must specify CASCADE to drop the parent table if a child table exists. When creating partitions as inheritance children, create auto dependency instead, so that partitions are dropped automatically when the parent is dropped i.e., without specifying CASCADE. --- src/backend/commands/tablecmds.c | 26 ++ src/test/regress/expected/alter_table.out | 10 -- src/test/regress/expected/create_table.out | 9 ++--- src/test/regress/expected/inherit.out | 22 ++ src/test/regress/expected/insert.out | 7 ++- src/test/regress/expected/update.out | 7 +-- src/test/regress/sql/alter_table.sql | 10 -- src/test/regress/sql/create_table.sql | 9 ++--- src/test/regress/sql/inherit.sql | 4 ++-- src/test/regress/sql/insert.sql| 7 ++- src/test/regress/sql/update.sql| 2 +- 11 files changed, 40 insertions(+), 73 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3cea220421..31b50ad77f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -289,9 +289,11 @@ static List *MergeAttributes(List *schema, List *supers, char relpersistence, static bool MergeCheckConstraint(List *constraints, char *name, Node *expr); static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel); static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel); -static void StoreCatalogInheritance(Oid relationId, List *supers); +static void StoreCatalogInheritance(Oid relationId, List *supers, + bool child_is_partition); static void StoreCatalogInheritance1(Oid relationId, Oid parentOid, - int16 seqNumber, Relation inhRelation); + int16 seqNumber, Relation inhRelation, + bool child_is_partition); static int findAttrByName(cons
Re: [HACKERS] dropping partitioned tables without CASCADE
On Tue, Feb 21, 2017 at 12:05 PM, Amit Langote wrote: > Hi Ashutosh, > > Thanks for taking a look at the patch. > > On 2017/02/20 21:49, Ashutosh Bapat wrote: >> Thanks for working on all the follow on work for partitioning feature. >> >> May be you should add all those patches in the next commitfest, so >> that we don't forget those. > > I think adding these as one of the PostgreSQL 10 Open Items [0] might be > better. I've done that. > >> On Mon, Feb 20, 2017 at 7:46 AM, Amit Langote wrote: >>> So I count more than a few votes saying that we should be able to DROP >>> partitioned tables without specifying CASCADE. >>> >>> I tried to implement that using the attached patch by having >>> StoreCatalogInheritance1() create DEPENDENCY_AUTO dependency between >>> parent and child if the child is a partition, instead of DEPENDENCY_NORMAL >>> that would otherwise be created. Now it seems that that is one way of >>> making sure that partitions are dropped when the root partitioned table is >>> dropped, not sure if the best; why create the pg_depend entries at all one >>> might ask. I chose it for now because that's the one with fewest lines of >>> change. Adjusted regression tests as well, since we recently tweaked >>> tests [1] to work around the irregularities of test output when using >>> CASCADE. >> >> The patch applies cleanly and regression does not show any failures. >> >> Here are some comments >> >> For the sake of readability you may want reverse the if and else order. >> -recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); >> +if (!child_is_partition) >> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); >> +else >> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); >> like >> +if (child_is_partition) >> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); >> +else >> +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); > > Sure, done. I still see -recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); +if (!child_is_partition) +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); +else +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); Are you sure you have attached the right patch? To avoid duplication you could actually write recordDependencyOn(&childobject, &parentobject, child_is_partition ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL); > >> It's weird that somebody can perform DROP TABLE on the partition without >> referring to its parent. That may be a useful feature as it allows one to >> detach the partition as well as remove the table in one command. But it looks >> wierd for someone familiar with partitioning features of other DBMSes. But >> then >> our partition creation command is CREATE TABLE So may be this is >> expected >> difference. > > There is a line on the CREATE TABLE page in the description of PARTITION > OF clause: > > "Note that dropping a partition with DROP TABLE requires taking an ACCESS > EXCLUSIVE lock on the parent table." > > In earlier proposals I had included the ALTER TABLE parent ADD/DROP > PARTITION commands, but CRAETE TABLE PARTITION OF / DROP TABLE prevailed. Ok. > >> --- cleanup: avoid using CASCADE >> -DROP TABLE list_parted, part_1; >> -DROP TABLE list_parted2, part_2, part_5, part_5_a; >> -DROP TABLE range_parted, part1, part2; >> +-- cleanup >> +DROP TABLE list_parted, list_parted2, range_parted; >> Testcases usually drop one table at a time, I guess, to reduce the >> differences >> when we add or remove tables from testcases. All such blocks should probably >> follow same policy. > > Hmm, I see this in src/test/regress/sql/inherit.sql:141 > > DROP TABLE firstparent, secondparent, jointchild, thirdparent, otherchild; Hmm, I can spot some more such usages. Let's keep this for the committer to decide. > >> drop table list_parted cascade; >> -NOTICE: drop cascades to 3 other objects >> -DETAIL: drop cascades to table part_ab_cd >> probably we should remove cascade from there, unless you are testing CASCADE >> functionality. Similarly for other blocks like >> drop table range_parted cascade; >> >> BTW, I noticed that although we are allowing foreign tables to be >> partitions, there are no tests in foreign_data.sql for testing it. If >> there would have been we would tests DROP TABLE on a partitioned table >> with foreign partitions as well. That file has testcases for testing >> foreign table inheritance, and should have tests for foreign table >> partitions. > > That makes sense. Patch 0002 is for that (I'm afraid this should be > posted separately though). I didn't add/repeat all the tests that were > added by the foreign table inheritance patch again for foreign partitions > (common inheritance rules apply to both cases), only added those for the > new partitioning commands and certain new rules. Thank
Re: [HACKERS] dropping partitioned tables without CASCADE
On 2017/02/21 15:35, Amit Langote wrote:
>> drop table list_parted cascade;
>> -NOTICE: drop cascades to 3 other objects
>> -DETAIL: drop cascades to table part_ab_cd
>> probably we should remove cascade from there, unless you are testing CASCADE
>> functionality. Similarly for other blocks like
>> drop table range_parted cascade;
Oops, failed to address this in the last email. Updated patches attached.
Thanks,
Amit
>From 0dc550d501805bcea81da9b2a06a39430427927c Mon Sep 17 00:00:00 2001
From: amit
Date: Thu, 16 Feb 2017 15:56:44 +0900
Subject: [PATCH 1/2] Allow dropping partitioned table without CASCADE
Currently, a normal dependency is created between a inheritance
parent and child when creating the child. That means one must
specify CASCADE to drop the parent table if a child table exists.
When creating partitions as inheritance children, create auto
dependency instead, so that partitions are dropped automatically
when the parent is dropped i.e., without specifying CASCADE.
---
src/backend/commands/tablecmds.c | 26 ++
src/test/regress/expected/alter_table.out | 10 --
src/test/regress/expected/create_table.out | 9 ++---
src/test/regress/expected/inherit.out | 22 ++
src/test/regress/expected/insert.out | 7 ++-
src/test/regress/expected/update.out | 7 +--
src/test/regress/sql/alter_table.sql | 10 --
src/test/regress/sql/create_table.sql | 9 ++---
src/test/regress/sql/inherit.sql | 4 ++--
src/test/regress/sql/insert.sql| 7 ++-
src/test/regress/sql/update.sql| 2 +-
11 files changed, 40 insertions(+), 73 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3cea220421..31b50ad77f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -289,9 +289,11 @@ static List *MergeAttributes(List *schema, List *supers, char relpersistence,
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
-static void StoreCatalogInheritance(Oid relationId, List *supers);
+static void StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation);
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition);
static int findAttrByName(const char *attributeName, List *schema);
static void AlterIndexNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
@@ -725,7 +727,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
typaddress);
/* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids);
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
/*
* We must bump the command counter to make the newly-created relation
@@ -2240,7 +2242,8 @@ MergeCheckConstraint(List *constraints, char *name, Node *expr)
* supers is a list of the OIDs of the new relation's direct ancestors.
*/
static void
-StoreCatalogInheritance(Oid relationId, List *supers)
+StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition)
{
Relation relation;
int16 seqNumber;
@@ -2270,7 +2273,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
{
Oid parentOid = lfirst_oid(entry);
- StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation);
+ StoreCatalogInheritance1(relationId, parentOid, seqNumber, relation,
+ child_is_partition);
seqNumber++;
}
@@ -2283,7 +2287,8 @@ StoreCatalogInheritance(Oid relationId, List *supers)
*/
static void
StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation)
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition)
{
TupleDesc desc = RelationGetDescr(inhRelation);
Datum values[Natts_pg_inherits];
@@ -2317,7 +2322,10 @@ StoreCatalogInheritance1(Oid relationId, Oid parentOid,
childobject.objectId = relationId;
childobject.objectSubId = 0;
- recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+ if (!child_is_partition)
+ recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL);
+ else
+ recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO);
/*
* Post creation hook of this inheritance. Since object_access_hook
@@ -10744,7 +10752,9 @@ CreateInheritance(Relation child_rel, Relation parent_rel)
StoreCatalogInheritance1(RelationGetRelid(child_rel),
RelationGetRelid(parent_rel),
inhseqno + 1,
- catalogRelation);
+ catalogRelation,
+ parent_rel->rd_rel
Re: [HACKERS] dropping partitioned tables without CASCADE
Hi Ashutosh, Thanks for taking a look at the patch. On 2017/02/20 21:49, Ashutosh Bapat wrote: > Thanks for working on all the follow on work for partitioning feature. > > May be you should add all those patches in the next commitfest, so > that we don't forget those. I think adding these as one of the PostgreSQL 10 Open Items [0] might be better. I've done that. > On Mon, Feb 20, 2017 at 7:46 AM, Amit Langote wrote: >> So I count more than a few votes saying that we should be able to DROP >> partitioned tables without specifying CASCADE. >> >> I tried to implement that using the attached patch by having >> StoreCatalogInheritance1() create DEPENDENCY_AUTO dependency between >> parent and child if the child is a partition, instead of DEPENDENCY_NORMAL >> that would otherwise be created. Now it seems that that is one way of >> making sure that partitions are dropped when the root partitioned table is >> dropped, not sure if the best; why create the pg_depend entries at all one >> might ask. I chose it for now because that's the one with fewest lines of >> change. Adjusted regression tests as well, since we recently tweaked >> tests [1] to work around the irregularities of test output when using >> CASCADE. > > The patch applies cleanly and regression does not show any failures. > > Here are some comments > > For the sake of readability you may want reverse the if and else order. > -recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); > +if (!child_is_partition) > +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); > +else > +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); > like > +if (child_is_partition) > +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); > +else > +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); Sure, done. > It's weird that somebody can perform DROP TABLE on the partition without > referring to its parent. That may be a useful feature as it allows one to > detach the partition as well as remove the table in one command. But it looks > wierd for someone familiar with partitioning features of other DBMSes. But > then > our partition creation command is CREATE TABLE So may be this is expected > difference. There is a line on the CREATE TABLE page in the description of PARTITION OF clause: "Note that dropping a partition with DROP TABLE requires taking an ACCESS EXCLUSIVE lock on the parent table." In earlier proposals I had included the ALTER TABLE parent ADD/DROP PARTITION commands, but CRAETE TABLE PARTITION OF / DROP TABLE prevailed. > --- cleanup: avoid using CASCADE > -DROP TABLE list_parted, part_1; > -DROP TABLE list_parted2, part_2, part_5, part_5_a; > -DROP TABLE range_parted, part1, part2; > +-- cleanup > +DROP TABLE list_parted, list_parted2, range_parted; > Testcases usually drop one table at a time, I guess, to reduce the differences > when we add or remove tables from testcases. All such blocks should probably > follow same policy. Hmm, I see this in src/test/regress/sql/inherit.sql:141 DROP TABLE firstparent, secondparent, jointchild, thirdparent, otherchild; > drop table list_parted cascade; > -NOTICE: drop cascades to 3 other objects > -DETAIL: drop cascades to table part_ab_cd > probably we should remove cascade from there, unless you are testing CASCADE > functionality. Similarly for other blocks like > drop table range_parted cascade; > > BTW, I noticed that although we are allowing foreign tables to be > partitions, there are no tests in foreign_data.sql for testing it. If > there would have been we would tests DROP TABLE on a partitioned table > with foreign partitions as well. That file has testcases for testing > foreign table inheritance, and should have tests for foreign table > partitions. That makes sense. Patch 0002 is for that (I'm afraid this should be posted separately though). I didn't add/repeat all the tests that were added by the foreign table inheritance patch again for foreign partitions (common inheritance rules apply to both cases), only added those for the new partitioning commands and certain new rules. Thanks, Amit [0] https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items >From c7cff17ab7861dbd0a4fb329115b3f99fd800325 Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 16 Feb 2017 15:56:44 +0900 Subject: [PATCH 1/2] Allow dropping partitioned table without CASCADE Currently, a normal dependency is created between a inheritance parent and child when creating the child. That means one must specify CASCADE to drop the parent table if a child table exists. When creating partitions as inheritance children, create auto dependency instead, so that partitions are dropped automatically when the parent is dropped i.e., without specifying CASCADE. --- src/backend/commands/tablecmds.c | 26 ++ src/test/regress/expected/alter_table.out | 10 -
Re: [HACKERS] dropping partitioned tables without CASCADE
Thanks for working on all the follow on work for partitioning feature. May be you should add all those patches in the next commitfest, so that we don't forget those. On Mon, Feb 20, 2017 at 7:46 AM, Amit Langote wrote: > Re-posting the patch I posted in a nearby thread [0]. > > On 2017/02/16 2:08, Robert Haas wrote: >> On Wed, Feb 15, 2017 at 11:34 AM, Alvaro Herrera >> wrote: >>> I think new-style partitioning is supposed to consider each partition as >>> an implementation detail of the table; the fact that you can manipulate >>> partitions separately does not really mean that they are their own >>> independent object. You don't stop to think "do I really want to drop >>> the TOAST table attached to this main table?" and attach a CASCADE >>> clause if so. You just drop the main table, and the toast one is >>> dropped automatically. I think new-style partitions should behave >>> equivalently. >> >> That's a reasonable point of view. I'd like to get some more opinions >> on this topic. I'm happy to have us do whatever most people want, but >> I'm worried that having table inheritance and table partitioning work >> differently will be create confusion. I'm also suspicious that there >> may be some implementation difficulties. On the hand, it does seem a >> little silly to say that DROP TABLE partitioned_table should always >> fail except in the degenerate case where there are no partitions, so >> maybe changing it is for the best. > > So I count more than a few votes saying that we should be able to DROP > partitioned tables without specifying CASCADE. > > I tried to implement that using the attached patch by having > StoreCatalogInheritance1() create DEPENDENCY_AUTO dependency between > parent and child if the child is a partition, instead of DEPENDENCY_NORMAL > that would otherwise be created. Now it seems that that is one way of > making sure that partitions are dropped when the root partitioned table is > dropped, not sure if the best; why create the pg_depend entries at all one > might ask. I chose it for now because that's the one with fewest lines of > change. Adjusted regression tests as well, since we recently tweaked > tests [1] to work around the irregularities of test output when using CASCADE. The patch applies cleanly and regression does not show any failures. Here are some comments For the sake of readability you may want reverse the if and else order. -recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); +if (!child_is_partition) +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); +else +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); like +if (child_is_partition) +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_AUTO); +else +recordDependencyOn(&childobject, &parentobject, DEPENDENCY_NORMAL); It's weird that somebody can perform DROP TABLE on the partition without referring to its parent. That may be a useful feature as it allows one to detach the partition as well as remove the table in one command. But it looks wierd for someone familiar with partitioning features of other DBMSes. But then our partition creation command is CREATE TABLE So may be this is expected difference. --- cleanup: avoid using CASCADE -DROP TABLE list_parted, part_1; -DROP TABLE list_parted2, part_2, part_5, part_5_a; -DROP TABLE range_parted, part1, part2; +-- cleanup +DROP TABLE list_parted, list_parted2, range_parted; Testcases usually drop one table at a time, I guess, to reduce the differences when we add or remove tables from testcases. All such blocks should probably follow same policy. drop table list_parted cascade; -NOTICE: drop cascades to 3 other objects -DETAIL: drop cascades to table part_ab_cd probably we should remove cascade from there, unless you are testing CASCADE functionality. Similarly for other blocks like drop table range_parted cascade; BTW, I noticed that although we are allowing foreign tables to be partitions, there are no tests in foreign_data.sql for testing it. If there would have been we would tests DROP TABLE on a partitioned table with foreign partitions as well. That file has testcases for testing foreign table inheritance, and should have tests for foreign table partitions. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] dropping partitioned tables without CASCADE
Re-posting the patch I posted in a nearby thread [0].
On 2017/02/16 2:08, Robert Haas wrote:
> On Wed, Feb 15, 2017 at 11:34 AM, Alvaro Herrera
> wrote:
>> I think new-style partitioning is supposed to consider each partition as
>> an implementation detail of the table; the fact that you can manipulate
>> partitions separately does not really mean that they are their own
>> independent object. You don't stop to think "do I really want to drop
>> the TOAST table attached to this main table?" and attach a CASCADE
>> clause if so. You just drop the main table, and the toast one is
>> dropped automatically. I think new-style partitions should behave
>> equivalently.
>
> That's a reasonable point of view. I'd like to get some more opinions
> on this topic. I'm happy to have us do whatever most people want, but
> I'm worried that having table inheritance and table partitioning work
> differently will be create confusion. I'm also suspicious that there
> may be some implementation difficulties. On the hand, it does seem a
> little silly to say that DROP TABLE partitioned_table should always
> fail except in the degenerate case where there are no partitions, so
> maybe changing it is for the best.
So I count more than a few votes saying that we should be able to DROP
partitioned tables without specifying CASCADE.
I tried to implement that using the attached patch by having
StoreCatalogInheritance1() create DEPENDENCY_AUTO dependency between
parent and child if the child is a partition, instead of DEPENDENCY_NORMAL
that would otherwise be created. Now it seems that that is one way of
making sure that partitions are dropped when the root partitioned table is
dropped, not sure if the best; why create the pg_depend entries at all one
might ask. I chose it for now because that's the one with fewest lines of
change. Adjusted regression tests as well, since we recently tweaked
tests [1] to work around the irregularities of test output when using CASCADE.
Thanks,
Amit
[0] https://postgr.es/m/ca132b99-0d18-439a-fe65-024085449259%40lab.ntt.co.jp
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c397814
>From 68133e249156a36be15a6e2e02f702e10f356db5 Mon Sep 17 00:00:00 2001
From: amit
Date: Thu, 16 Feb 2017 15:56:44 +0900
Subject: [PATCH] Allow dropping partitioned table without CASCADE
Currently, a normal dependency is created between a inheritance
parent and child when creating the child. That means one must
specify CASCADE to drop the parent table if a child table exists.
When creating partitions as inheritance children, create auto
dependency instead, so that partitions are dropped automatically
when the parent is dropped i.e., without specifying CASCADE.
---
src/backend/commands/tablecmds.c | 26 ++
src/test/regress/expected/alter_table.out | 10 --
src/test/regress/expected/create_table.out | 9 ++---
src/test/regress/expected/inherit.out | 18 --
src/test/regress/expected/insert.out | 7 ++-
src/test/regress/expected/update.out | 5 -
src/test/regress/sql/alter_table.sql | 10 --
src/test/regress/sql/create_table.sql | 9 ++---
src/test/regress/sql/insert.sql| 7 ++-
9 files changed, 34 insertions(+), 67 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3cea220421..31b50ad77f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -289,9 +289,11 @@ static List *MergeAttributes(List *schema, List *supers, char relpersistence,
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
-static void StoreCatalogInheritance(Oid relationId, List *supers);
+static void StoreCatalogInheritance(Oid relationId, List *supers,
+ bool child_is_partition);
static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
- int16 seqNumber, Relation inhRelation);
+ int16 seqNumber, Relation inhRelation,
+ bool child_is_partition);
static int findAttrByName(const char *attributeName, List *schema);
static void AlterIndexNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
@@ -725,7 +727,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
typaddress);
/* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids);
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
/*
* We must bump the command counter to make the newly-created relation
@@ -2240,7 +2242,8 @@ MergeCheckConstraint(List *constraints, char *name, Node *expr)
* supers is a list of the OIDs of the new relation's direct ancestors.
*/
static void
-StoreCatalog
