Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-08 Thread Amit Langote
On 2017/05/08 12:42, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> Thanks for committing the patch after improving it quite a bit, and sorry
>> that I couldn't reply promptly during the last week due to vacation.
> 
> No worries, hopefully you have an opportunity to review the additional
> changes I made and understand why they were necessary.  Certainly, feel
> free to reach out if you have any questions or notice anything else
> which should be improved.

Do you intend to push the other patch to add regression tests for the
non-inherited constraints?  Here it is attached again for you to look over.

Thanks,
Amit
>From a27689d445d88462dc499f04ed6779cb686a9588 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 18 Apr 2017 10:59:35 +0900
Subject: [PATCH] Improve test coverage of partition constraints

Better exercise the code manipulating partition constraints a bit
differently from the traditional inheritance children.
---
 src/bin/pg_dump/t/002_pg_dump.pl   | 10 +---
 src/test/regress/expected/create_table.out | 41 ++
 src/test/regress/sql/create_table.sql  | 21 ---
 3 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ce0c9ef54d..08ae7eb83b 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4791,12 +4791,16 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 		catch_all=> 'CREATE ... commands',
 		create_order => 91,
 		create_sql => 'CREATE TABLE dump_test_second_schema.measurement_y2006m2
-	   PARTITION OF dump_test.measurement FOR VALUES
-	   FROM (\'2006-02-01\') TO (\'2006-03-01\');',
+	   PARTITION OF dump_test.measurement (
+	  unitsales DEFAULT 0 CHECK (unitsales >= 0)
+	   )
+	   FOR VALUES FROM (\'2006-02-01\') TO (\'2006-03-01\');',
 		regexp => qr/^
 			\Q-- Name: measurement_y2006m2;\E.*\n
 			\Q--\E\n\n
-			\QCREATE TABLE measurement_y2006m2 PARTITION OF dump_test.measurement\E\n
+			\QCREATE TABLE measurement_y2006m2 PARTITION OF dump_test.measurement (\E\n
+			\s+\QCONSTRAINT measurement_y2006m2_unitsales_check CHECK ((unitsales >= 0))\E\n
+			\)\n
 			\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
 			/xm,
 		like => {
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index dda0d7ee5d..79603166d1 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -618,16 +618,42 @@ CREATE TABLE part_b PARTITION OF parted (
 ) FOR VALUES IN ('b');
 ERROR:  column "b" specified more than once
 CREATE TABLE part_b PARTITION OF parted (
-	b NOT NULL DEFAULT 1 CHECK (b >= 0),
-	CONSTRAINT check_a CHECK (length(a) > 0)
+	b NOT NULL DEFAULT 1,
+	CONSTRAINT check_a CHECK (length(a) > 0),
+	CONSTRAINT check_b CHECK (b >= 0)
 ) FOR VALUES IN ('b');
 NOTICE:  merging constraint "check_a" with inherited definition
 -- conislocal should be false for any merged constraints
-SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass AND conname = 'check_a';
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
  conislocal | coninhcount 
 +-
  f  |   1
-(1 row)
+ t  |   0
+(2 rows)
+
+-- Once check_b is added to the parent, it should be made non-local for part_b
+ALTER TABLE parted ADD CONSTRAINT check_b CHECK (b >= 0);
+NOTICE:  merging constraint "check_b" with inherited definition
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
+ conislocal | coninhcount 
++-
+ f  |   1
+ f  |   1
+(2 rows)
+
+-- Neither check_a nor check_b are droppable from part_b
+ALTER TABLE part_b DROP CONSTRAINT check_a;
+ERROR:  cannot drop inherited constraint "check_a" of relation "part_b"
+ALTER TABLE part_b DROP CONSTRAINT check_b;
+ERROR:  cannot drop inherited constraint "check_b" of relation "part_b"
+-- And dropping it from parted should leave no trace of them on part_b, unlike
+-- traditional inheritance where they will be left behind, because they would
+-- be local constraints.
+ALTER TABLE parted DROP CONSTRAINT check_a, DROP CONSTRAINT check_b;
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
+ conislocal | coninhcount 
++-
+(0 rows)
 
 -- specify PARTITION BY for a partition
 CREATE TABLE fail_part_col_not_found PARTITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
@@ -643,9 +669,6 @@ CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
  a  | text|   |  | 
  b  | integer |   | not null | 1
 Partition of: parted FOR VALUES IN ('b')
-Check constraints:
-"check_a" 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-07 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> Thanks for committing the patch after improving it quite a bit, and sorry
> that I couldn't reply promptly during the last week due to vacation.

No worries, hopefully you have an opportunity to review the additional
changes I made and understand why they were necessary.  Certainly, feel
free to reach out if you have any questions or notice anything else
which should be improved.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-07 Thread Amit Langote
Hi Stephen,

On 2017/05/06 12:28, Stephen Frost wrote:
> Noah,
> 
> On Fri, May 5, 2017 at 23:19 Noah Misch  wrote:
> 
>> On Thu, May 04, 2017 at 05:47:02PM -0400, Stephen Frost wrote:
>>> * Amit Langote (amitlangot...@gmail.com) wrote:
 On Wed, May 3, 2017 at 12:05 PM, Stephen Frost 
>> wrote:
> Assuming this looks good to you, I'll push it tomorrow, possibly with
> other minor adjustments and perhaps a few more tests.

 Your latest patch looks good to me.
>>>
>>> Found a few more issues (like pg_dump not working against older versions
>>> of PG, because the queries for older versions hadn't been adjusted) and
>>> did a bit more tidying.
>>>
>>> I'll push this in a couple of hours.
>>
>> This PostgreSQL 10 open item is past due for your status update.  Kindly
>> send
>> a status update within 24 hours, and include a date for your subsequent
>> status
>> update.  Refer to the policy on open item ownership:
>>
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>>
>> I see you did commit a related patch.  Is this item done?
> 
> 
> Yes, I believe it to be. I anticipate reviewing the code in this area more
> in the coming weeks, but this specific issue can be marked as resolved.

Thanks for committing the patch after improving it quite a bit, and sorry
that I couldn't reply promptly during the last week due to vacation.

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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-05 Thread Stephen Frost
Noah,

On Fri, May 5, 2017 at 23:19 Noah Misch  wrote:

> On Thu, May 04, 2017 at 05:47:02PM -0400, Stephen Frost wrote:
> > * Amit Langote (amitlangot...@gmail.com) wrote:
> > > On Wed, May 3, 2017 at 12:05 PM, Stephen Frost 
> wrote:
> > > > Assuming this looks good to you, I'll push it tomorrow, possibly with
> > > > other minor adjustments and perhaps a few more tests.
> > >
> > > Your latest patch looks good to me.
> >
> > Found a few more issues (like pg_dump not working against older versions
> > of PG, because the queries for older versions hadn't been adjusted) and
> > did a bit more tidying.
> >
> > I'll push this in a couple of hours.
>
> This PostgreSQL 10 open item is past due for your status update.  Kindly
> send
> a status update within 24 hours, and include a date for your subsequent
> status
> update.  Refer to the policy on open item ownership:
>
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>
> I see you did commit a related patch.  Is this item done?


Yes, I believe it to be. I anticipate reviewing the code in this area more
in the coming weeks, but this specific issue can be marked as resolved.

Thanks!

Stephen


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-05 Thread Noah Misch
On Thu, May 04, 2017 at 05:47:02PM -0400, Stephen Frost wrote:
> * Amit Langote (amitlangot...@gmail.com) wrote:
> > On Wed, May 3, 2017 at 12:05 PM, Stephen Frost  wrote:
> > > Assuming this looks good to you, I'll push it tomorrow, possibly with
> > > other minor adjustments and perhaps a few more tests.
> > 
> > Your latest patch looks good to me.
> 
> Found a few more issues (like pg_dump not working against older versions
> of PG, because the queries for older versions hadn't been adjusted) and
> did a bit more tidying.
> 
> I'll push this in a couple of hours.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

I see you did commit a related patch.  Is this item done?


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-04 Thread Stephen Frost
Amit,

* Amit Langote (amitlangot...@gmail.com) wrote:
> On Wed, May 3, 2017 at 12:05 PM, Stephen Frost  wrote:
> > Assuming this looks good to you, I'll push it tomorrow, possibly with
> > other minor adjustments and perhaps a few more tests.
> 
> Your latest patch looks good to me.

Found a few more issues (like pg_dump not working against older versions
of PG, because the queries for older versions hadn't been adjusted) and
did a bit more tidying.

I'll push this in a couple of hours.

Thanks!

Stephen
From ec25e5ecf44d461a93187999d3a491eae49b587f Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 4 May 2017 17:14:51 -0400
Subject: [PATCH] Change the way pg_dump retrieves partitioning info

This gets rid of the code that issued separate queries to retrieve the
partitioning parent-child relationship, parent partition key, and child
partition bound information.  With this patch, the information is
retrieved instead using the queries issued from getTables() and
getInherits(), which is both more efficient than the previous approach
and doesn't require any new code.

Since the partitioning parent-child relationship is now retrieved with
the same old code that handles inheritance, partition attributes receive
a proper flagInhAttrs() treatment (that it didn't receive before), which
is needed so that the inherited NOT NULL constraints are not emitted if
we already emitted it for the parent.

Also, fix a bug in pg_dump's --binary-upgrade code, which caused pg_dump
to emit invalid command to attach a partition to its parent.

Author: Amit Langote, with some additional changes by me.
---
 src/bin/pg_dump/common.c |  90 -
 src/bin/pg_dump/pg_dump.c| 264 +--
 src/bin/pg_dump/pg_dump.h|  15 +--
 src/bin/pg_dump/t/002_pg_dump.pl |  36 +-
 4 files changed, 153 insertions(+), 252 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index e2bc357..47191be 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -68,8 +68,6 @@ static int	numextmembers;
 
 static void flagInhTables(TableInfo *tbinfo, int numTables,
 			  InhInfo *inhinfo, int numInherits);
-static void flagPartitions(TableInfo *tblinfo, int numTables,
-			  PartInfo *partinfo, int numPartitions);
 static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables);
 static DumpableObject **buildIndexArray(void *objArray, int numObjs,
 Size objSize);
@@ -77,8 +75,6 @@ static int	DOCatalogIdCompare(const void *p1, const void *p2);
 static int	ExtensionMemberIdCompare(const void *p1, const void *p2);
 static void findParentsByOid(TableInfo *self,
  InhInfo *inhinfo, int numInherits);
-static void findPartitionParentByOid(TableInfo *self, PartInfo *partinfo,
- int numPartitions);
 static int	strInArray(const char *pattern, char **arr, int arr_size);
 
 
@@ -97,10 +93,8 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	NamespaceInfo *nspinfo;
 	ExtensionInfo *extinfo;
 	InhInfo*inhinfo;
-	PartInfo*partinfo;
 	int			numAggregates;
 	int			numInherits;
-	int			numPartitions;
 	int			numRules;
 	int			numProcLangs;
 	int			numCasts;
@@ -238,10 +232,6 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	inhinfo = getInherits(fout, );
 
 	if (g_verbose)
-		write_msg(NULL, "reading partition information\n");
-	partinfo = getPartitions(fout, );
-
-	if (g_verbose)
 		write_msg(NULL, "reading event triggers\n");
 	getEventTriggers(fout, );
 
@@ -255,11 +245,6 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 		write_msg(NULL, "finding inheritance relationships\n");
 	flagInhTables(tblinfo, numTables, inhinfo, numInherits);
 
-	/* Link tables to partition parents, mark parents as interesting */
-	if (g_verbose)
-		write_msg(NULL, "finding partition relationships\n");
-	flagPartitions(tblinfo, numTables, partinfo, numPartitions);
-
 	if (g_verbose)
 		write_msg(NULL, "reading column info for interesting tables\n");
 	getTableAttrs(fout, tblinfo, numTables);
@@ -293,10 +278,6 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	getPolicies(fout, tblinfo, numTables);
 
 	if (g_verbose)
-		write_msg(NULL, "reading partition key information for interesting tables\n");
-	getTablePartitionKeyInfo(fout, tblinfo, numTables);
-
-	if (g_verbose)
 		write_msg(NULL, "reading publications\n");
 	getPublications(fout);
 
@@ -354,43 +335,6 @@ flagInhTables(TableInfo *tblinfo, int numTables,
 	}
 }
 
-/* flagPartitions -
- *	 Fill in parent link fields of every target table that is partition,
- *	 and mark parents of partitions as interesting
- *
- * modifies tblinfo
- */
-static void
-flagPartitions(TableInfo *tblinfo, int numTables,
-			  PartInfo *partinfo, int numPartitions)
-{
-	int		i;
-
-	for (i = 0; i < numTables; i++)
-	{
-		/* Some kinds are never partitions */
-		if (tblinfo[i].relkind == RELKIND_SEQUENCE ||
-			tblinfo[i].relkind == RELKIND_VIEW ||
-			tblinfo[i].relkind == 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-03 Thread Stephen Frost
Amit,

* Amit Langote (amitlangot...@gmail.com) wrote:
> On Wed, May 3, 2017 at 12:05 PM, Stephen Frost  wrote:
> > * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> >> Attached updated patches.
> >
> > Please find an updated version which corrects the issue with
> > binary-upgrade of partitioned tables having partitions in other schemas,
> > along with a few other minor improvements.
> >
> > If you could take a look at it, I'd appreciate it.  We already had a
> > test case in the pg_dump TAP tests for partitions existing in a schema
> > different from the partitioned table, but we weren't checking the
> > binary-upgrade case, so I've added a check to do that now.  I'm sure
> > additional tests would be good to add and will take a look at doing that
> > tomorrow, but this hopefully closes at least the latest issue.
> >
> > Assuming this looks good to you, I'll push it tomorrow, possibly with
> > other minor adjustments and perhaps a few more tests.
> 
> Your latest patch looks good to me.

Ok, great, thanks for taking a look at it.  Too late for me to push it
tonight, so I'll do so tomorrow.

Thanks again!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-03 Thread Amit Langote
Hi Stephen,

On Wed, May 3, 2017 at 12:05 PM, Stephen Frost  wrote:
> Amit,
>
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> Attached updated patches.
>
> Please find an updated version which corrects the issue with
> binary-upgrade of partitioned tables having partitions in other schemas,
> along with a few other minor improvements.
>
> If you could take a look at it, I'd appreciate it.  We already had a
> test case in the pg_dump TAP tests for partitions existing in a schema
> different from the partitioned table, but we weren't checking the
> binary-upgrade case, so I've added a check to do that now.  I'm sure
> additional tests would be good to add and will take a look at doing that
> tomorrow, but this hopefully closes at least the latest issue.
>
> Assuming this looks good to you, I'll push it tomorrow, possibly with
> other minor adjustments and perhaps a few more tests.

Your latest patch looks good to me.

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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-02 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> Attached updated patches.

Please find an updated version which corrects the issue with
binary-upgrade of partitioned tables having partitions in other schemas,
along with a few other minor improvements.

If you could take a look at it, I'd appreciate it.  We already had a
test case in the pg_dump TAP tests for partitions existing in a schema
different from the partitioned table, but we weren't checking the
binary-upgrade case, so I've added a check to do that now.  I'm sure
additional tests would be good to add and will take a look at doing that
tomorrow, but this hopefully closes at least the latest issue.

Assuming this looks good to you, I'll push it tomorrow, possibly with
other minor adjustments and perhaps a few more tests.

Thanks!

Stephen
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
new file mode 100644
index e2bc357..47191be
*** a/src/bin/pg_dump/common.c
--- b/src/bin/pg_dump/common.c
*** static int	numextmembers;
*** 68,75 
  
  static void flagInhTables(TableInfo *tbinfo, int numTables,
  			  InhInfo *inhinfo, int numInherits);
- static void flagPartitions(TableInfo *tblinfo, int numTables,
- 			  PartInfo *partinfo, int numPartitions);
  static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables);
  static DumpableObject **buildIndexArray(void *objArray, int numObjs,
  Size objSize);
--- 68,73 
*** static int	DOCatalogIdCompare(const void
*** 77,84 
  static int	ExtensionMemberIdCompare(const void *p1, const void *p2);
  static void findParentsByOid(TableInfo *self,
   InhInfo *inhinfo, int numInherits);
- static void findPartitionParentByOid(TableInfo *self, PartInfo *partinfo,
-  int numPartitions);
  static int	strInArray(const char *pattern, char **arr, int arr_size);
  
  
--- 75,80 
*** getSchemaData(Archive *fout, int *numTab
*** 97,106 
  	NamespaceInfo *nspinfo;
  	ExtensionInfo *extinfo;
  	InhInfo*inhinfo;
- 	PartInfo*partinfo;
  	int			numAggregates;
  	int			numInherits;
- 	int			numPartitions;
  	int			numRules;
  	int			numProcLangs;
  	int			numCasts;
--- 93,100 
*** getSchemaData(Archive *fout, int *numTab
*** 238,247 
  	inhinfo = getInherits(fout, );
  
  	if (g_verbose)
- 		write_msg(NULL, "reading partition information\n");
- 	partinfo = getPartitions(fout, );
- 
- 	if (g_verbose)
  		write_msg(NULL, "reading event triggers\n");
  	getEventTriggers(fout, );
  
--- 232,237 
*** getSchemaData(Archive *fout, int *numTab
*** 255,265 
  		write_msg(NULL, "finding inheritance relationships\n");
  	flagInhTables(tblinfo, numTables, inhinfo, numInherits);
  
- 	/* Link tables to partition parents, mark parents as interesting */
- 	if (g_verbose)
- 		write_msg(NULL, "finding partition relationships\n");
- 	flagPartitions(tblinfo, numTables, partinfo, numPartitions);
- 
  	if (g_verbose)
  		write_msg(NULL, "reading column info for interesting tables\n");
  	getTableAttrs(fout, tblinfo, numTables);
--- 245,250 
*** getSchemaData(Archive *fout, int *numTab
*** 293,302 
  	getPolicies(fout, tblinfo, numTables);
  
  	if (g_verbose)
- 		write_msg(NULL, "reading partition key information for interesting tables\n");
- 	getTablePartitionKeyInfo(fout, tblinfo, numTables);
- 
- 	if (g_verbose)
  		write_msg(NULL, "reading publications\n");
  	getPublications(fout);
  
--- 278,283 
*** flagInhTables(TableInfo *tblinfo, int nu
*** 354,396 
  	}
  }
  
- /* flagPartitions -
-  *	 Fill in parent link fields of every target table that is partition,
-  *	 and mark parents of partitions as interesting
-  *
-  * modifies tblinfo
-  */
- static void
- flagPartitions(TableInfo *tblinfo, int numTables,
- 			  PartInfo *partinfo, int numPartitions)
- {
- 	int		i;
- 
- 	for (i = 0; i < numTables; i++)
- 	{
- 		/* Some kinds are never partitions */
- 		if (tblinfo[i].relkind == RELKIND_SEQUENCE ||
- 			tblinfo[i].relkind == RELKIND_VIEW ||
- 			tblinfo[i].relkind == RELKIND_MATVIEW)
- 			continue;
- 
- 		/* Don't bother computing anything for non-target tables, either */
- 		if (!tblinfo[i].dobj.dump)
- 			continue;
- 
- 		/* Find the parent TableInfo and save */
- 		findPartitionParentByOid([i], partinfo, numPartitions);
- 
- 		/* Mark the parent as interesting for getTableAttrs */
- 		if (tblinfo[i].partitionOf)
- 		{
- 			tblinfo[i].partitionOf->interesting = true;
- 			addObjectDependency([i].dobj,
- tblinfo[i].partitionOf->dobj.dumpId);
- 		}
- 	}
- }
- 
  /* flagInhAttrs -
   *	 for each dumpable table in tblinfo, flag its inherited attributes
   *
--- 335,340 
*** findParentsByOid(TableInfo *self,
*** 992,1031 
  }
  
  /*
-  * findPartitionParentByOid
-  *	  find a partition's parent in tblinfo[]
-  */
- static void
- findPartitionParentByOid(TableInfo *self, PartInfo *partinfo,
- 		 int numPartitions)
- {
- 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-02 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> Now that WITH OPTIONS is optional even for CREATE TABLE OF, perhaps it
> needs to be mentioned in the release notes?

Doesn't strike me as rising to the level of needing to go into the
release notes, but I won't object if people feel that it needs
mentioning.

> Attached updated patches.

Thanks, but aren't the changes for handling pg_dump --binary-upgrade
when dealing with partitions whose parents are in another schema
backwards?

The source schema selected is for the partition, so we don't need to
schema-qualify the partition, but we do need to schema-qualify the
parent because it could be in another schema.  I think the approach to
use is to decide, first, if we need to schema-qualify the parent or not,
then set up a string which has the qualified (if necessary) name of the
parent, and then just use that when appropriate while building the ALTER
TABLE command.  Remember, we select the source schema of the table in
tbinfo at the top of dumpTableSchema() (see 'selectSourceSchema()'), so
it shouldn't ever be necessary to schema-qualify the table in tbinfo.

I've somehow managed to run out of time again today (it's gotten quite
busy lately), but I'll try to find time late tonight to continue working
on this, or I'll be working on it again tomorrow.  This *really* needs
some tests that actually cover this case, as it's clearly not hard to
get confused about what needs to be qualified and what doesn't.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-05-01 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> Sorry about the delay.

No worries, I'm just back from being in NY and will take a look at this
tomorrow (wrt the open item, I'll provide a status tomorrow).

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-30 Thread Amit Langote
Hi Stephen,

Sorry about the delay.

On 2017/04/27 23:17, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
 So to summarize what the patches do (some of these were posted earlier)

 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
>>>
>>> I'm trying to understand why this is also different.  At least on an
>>> initial look, there seems to be a bunch more copy-and-paste from the
>>> existing TypedTable to Partition in gram.y, which seems to all boil down
>>> to removing the need for WITH OPTIONS to be specified.  If WITH OPTIONS
>>> would be too much to have included for partitioning, and it isn't
>>> actually necessary, why not just make it optional, and use the same
>>> construct for both, removing all the duplicaiton and the need for
>>> pg_dump to output it?
>>
>> OK, I think optionally allowing WITH OPTIONS keywords would be nice.
>>
>> So in lieu of this patch, I propose a patch that modifies gram.y to allow
>> WITH OPTIONS to specified.
> 
> The point I was trying to get at was that if you make WITH OPTIONS
> optional for the TypedTableElement case, you can remove all of the
> PartitionElement-related productions and then both the OF type_name and
> the PARTITION OF case will accept WITH OPTIONS as noise words and also
> work without WITH OPTIONS being specified.
> 
> This also makes the code actually match the documentation since, at
> least in the CREATE FOREIGN TABLE documentation, we claim that WITH
> OPTIONS is required.
> 
> Please see a proposed patch attached to accomplish this.

The committed patch looks good to me.  Thanks.

Now that WITH OPTIONS is optional even for CREATE TABLE OF, perhaps it
needs to be mentioned in the release notes?

>>> Given that we're now setting numParents for partitions, I
>>> would think we'd just move the if (tbinfo->partitionOf) branches up
>>> under the if (numParents > 0) branches, which I think would also help us
>>> catch additional issues, like the fact that a binary-upgrade with
>>> partitions in a different namespace from the partitioned table would
>>> fail because the ATTACH PARTITION code doesn't account for it:
>>>
>>> appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
>>>   fmtId(tbinfo->partitionOf->dobj.name));
>>> appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
>>>   fmtId(tbinfo->dobj.name),
>>>   tbinfo->partitiondef);
>>>
>>> unlike the existing inheritance code a few lines above, which does:
>>>
>>> appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
>>>   fmtId(tbinfo->dobj.name));
>>> if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
>>> appendPQExpBuffer(q, "%s.",
>>> 
>>> fmtId(parentRel->dobj.namespace->dobj.name));
>>> appendPQExpBuffer(q, "%s;\n",
>>>   fmtId(parentRel->dobj.name));
>>
>> That's a good point.  I put both cases under the if (numParents > 0) block
>> and appropriate text is emitted depending on whether the table is a
>> partition or plain inheritance child.
> 
> Right, ok.
> 
 0004: Change the way pg_dump retrieves partitioning info (removes a lot
   of unnecessary code and instead makes partitioning case use the old
   inheritance code, etc.)
>>>
>>> This looks pretty good, at least on first blush, probably in part
>>> because it's mostly removing code.  The CASE statement in the
>>> getTables() query isn't needed, once pg_get_partkeydef() has been
>>> changed to not throw an ERROR when passed a non-partitioned table OID.
>>
>> Oh yes, fixed.
> 
> Good.
> 
>> 0003: Change the way pg_dump retrieves partitioning info (removes a lot
>>   of unnecessary code and instead makes partitioning case use the old
>>   inheritance code, etc.)
> 
> This has conflicts due to my proposed patch as my patch changes pg_dump
> to not output the now-noise-words WITH OPTIONS at all.

Rebased.

>> 0004: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
>>   INHERIT to be emitted to attach a partition in addition to of ALTER
>>   TABLE ATTACH PARTITION and that no schema-name was emitted where it
>>   should have
> 
> Given that it's touching the same places, this would really make sense
> to merge into 0003 now.

OK, done.

> I'm going to continue to mull over the attached for a bit and add some
> tests to it, but if it looks good then I'll push it this afternoon,
> after which you'll hopefully have time to rebase 0003 and merge in 0004
> to that, which I can review and probably push tomorrow.

Attached updated patches.

Thanks,
Amit
>From 41d8aa34c6a780fb3f46dc19f41b901e89be8726 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 18 Apr 2017 10:59:35 +0900
Subject: [PATCH 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-27 Thread Stephen Frost
Noah,

* Noah Misch (n...@leadboat.com) wrote:
> On Sun, Apr 23, 2017 at 11:58:23PM +, Stephen Frost wrote:
> > The status is simply that I've been considering Robert's comments regarding
> > the documentation and have had a busy weekend. I'll provide an update
> > tomorrow.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:

I committed the immediate issue regarding the ALTER TABLE ONLY commands
failing when pg_dump's output was run against a new system on Tuesday,
so that part is "fixed", but I continue to work through the lingering
related issues with Amit and have been pushed related patches both
yesterday and today.

I'm anticipating a new patch (or perhaps two) from Amit tomorrow (well,
later today at this point...) that I hope to push to clean up other
related bugs in pg_dump's handling of partitioned tables.

Frankly, I fully expect to find additional issues beyond those as we
move forward, but assuming I can finish up the last couple patches from
Amit tomorrow then I'll close out this open issue and open new ones as
new issues are discovered.  I'll commit to having this issue closed out,
barring additional issues, by Sunday (2017-04-30).

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-27 Thread Noah Misch
On Sun, Apr 23, 2017 at 11:58:23PM +, Stephen Frost wrote:
> Noah, all,
> 
> On Sun, Apr 23, 2017 at 19:52 Noah Misch  wrote:
> 
> > On Sat, Apr 22, 2017 at 01:14:08PM -0700, Noah Misch wrote:
> > > On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote:
> > > > * Noah Misch (n...@leadboat.com) wrote:
> > > > > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote:
> > > > > > I've put up a new patch for review on the thread and plan to commit
> > > > > > that tomorrow, assuming there isn't anything further.  That should
> > > > > > resolve the immediate issue, but I do think there's some merit to
> > Amit's
> > > > > > follow-on patches and will continue to discuss those and may
> > commit one
> > > > > > or both of those later this week.
> > > > >
> > > > > This PostgreSQL 10 open item is past due for your status update.
> > Kindly send
> > > > > a status update within 24 hours, and include a date for your
> > subsequent status
> > > > > update.  Refer to the policy on open item ownership:
> > > > >
> > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> > > >
> > > > I thought I'd be able to get to this today, but other activities have
> > > > taken up the time I had planned to work on this.  I'll be on it again
> > > > tomorrow morning and will provide an update (most likely a couple of
> > > > commits) by COB tomorrow.
> > >
> > > This PostgreSQL 10 open item is again past due for your status update.
> > Kindly
> > > send a status update within 24 hours, and include a date for your
> > subsequent
> > > status update.  Refer to the policy on open item ownership:
> > >
> > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> >
> > IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past
> > due
> > for your status update.  Please reacquaint yourself with the policy on open
> > item ownership[1] and then reply immediately.  If I do not hear from you by
> > 2017-04-25 00:00 UTC, I will transfer this item to release management team
> > ownership without further notice.
> 
> 
> The status is simply that I've been considering Robert's comments regarding
> the documentation and have had a busy weekend. I'll provide an update
> tomorrow.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-27 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> >> So to summarize what the patches do (some of these were posted earlier)
> >>
> >> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
> > 
> > I'm trying to understand why this is also different.  At least on an
> > initial look, there seems to be a bunch more copy-and-paste from the
> > existing TypedTable to Partition in gram.y, which seems to all boil down
> > to removing the need for WITH OPTIONS to be specified.  If WITH OPTIONS
> > would be too much to have included for partitioning, and it isn't
> > actually necessary, why not just make it optional, and use the same
> > construct for both, removing all the duplicaiton and the need for
> > pg_dump to output it?
> 
> OK, I think optionally allowing WITH OPTIONS keywords would be nice.
> 
> So in lieu of this patch, I propose a patch that modifies gram.y to allow
> WITH OPTIONS to specified.

The point I was trying to get at was that if you make WITH OPTIONS
optional for the TypedTableElement case, you can remove all of the
PartitionElement-related productions and then both the OF type_name and
the PARTITION OF case will accept WITH OPTIONS as noise words and also
work without WITH OPTIONS being specified.

This also makes the code actually match the documentation since, at
least in the CREATE FOREIGN TABLE documentation, we claim that WITH
OPTIONS is required.

Please see a proposed patch attached to accomplish this.

> > Given that we're now setting numParents for partitions, I
> > would think we'd just move the if (tbinfo->partitionOf) branches up
> > under the if (numParents > 0) branches, which I think would also help us
> > catch additional issues, like the fact that a binary-upgrade with
> > partitions in a different namespace from the partitioned table would
> > fail because the ATTACH PARTITION code doesn't account for it:
> > 
> > appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
> >   fmtId(tbinfo->partitionOf->dobj.name));
> > appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
> >   fmtId(tbinfo->dobj.name),
> >   tbinfo->partitiondef);
> > 
> > unlike the existing inheritance code a few lines above, which does:
> > 
> > appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
> >   fmtId(tbinfo->dobj.name));
> > if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
> > appendPQExpBuffer(q, "%s.",
> > 
> > fmtId(parentRel->dobj.namespace->dobj.name));
> > appendPQExpBuffer(q, "%s;\n",
> >   fmtId(parentRel->dobj.name));
> 
> That's a good point.  I put both cases under the if (numParents > 0) block
> and appropriate text is emitted depending on whether the table is a
> partition or plain inheritance child.

Right, ok.

> >> 0004: Change the way pg_dump retrieves partitioning info (removes a lot
> >>   of unnecessary code and instead makes partitioning case use the old
> >>   inheritance code, etc.)
> > 
> > This looks pretty good, at least on first blush, probably in part
> > because it's mostly removing code.  The CASE statement in the
> > getTables() query isn't needed, once pg_get_partkeydef() has been
> > changed to not throw an ERROR when passed a non-partitioned table OID.
> 
> Oh yes, fixed.

Good.

> 0003: Change the way pg_dump retrieves partitioning info (removes a lot
>   of unnecessary code and instead makes partitioning case use the old
>   inheritance code, etc.)

This has conflicts due to my proposed patch as my patch changes pg_dump
to not output the now-noise-words WITH OPTIONS at all.

> 0004: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
>   INHERIT to be emitted to attach a partition in addition to of ALTER
>   TABLE ATTACH PARTITION and that no schema-name was emitted where it
>   should have

Given that it's touching the same places, this would really make sense
to merge into 0003 now.

I'm going to continue to mull over the attached for a bit and add some
tests to it, but if it looks good then I'll push it this afternoon,
after which you'll hopefully have time to rebase 0003 and merge in 0004
to that, which I can review and probably push tomorrow.

Thanks!

Stephen
From 1d997077fb4e5395fed0e8e0dd7dc3b629a11553 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 27 Apr 2017 09:19:34 -0400
Subject: [PATCH] Remove unnecessairly duplicated gram.y productions

Declarative partitioning duplicated the TypedTableElement productions,
evidently to remove the need to specify WITH OPTIONS when creating
partitions.  Instead, simply make WITH OPTIONS optional in the
TypedTableElement production and remove all of the duplicate
PartitionElement-related productions.  

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-27 Thread Amit Langote
Hi Stephen,

On 2017/04/26 23:31, Stephen Frost wrote:
>>> I looked through
>>> pg_get_partkeydef() and it didn't seem to be particularly expensive to
>>> run, though evidently it doesn't handle being passed an OID that it
>>> doesn't expect very cleanly:
>>>
>>> =# select pg_get_partkeydef(oid) from pg_class;
>>> ERROR:  cache lookup failed for partition key of 52333
>>>
>>> I don't believe that's really very appropriate of it to do though and
>>> instead it should work the same way pg_get_indexdef() and others do and
>>> return NULL in such cases, so people can use it sanely.
>>>
>>> I'm working through this but it's going to take a bit of time to clean
>>> it up and it's not going to get finished today, but I do think it needs
>>> to get done and so I'll work to make it happen this week.  I didn't
>>> anticipate finding this, obviously and am a bit disappointed by it.
>>
>> Yeah, that was sloppy. :-(
>>
>> Attached patch 0005 fixes that and adds a test to rules.sql.
> 
> Good, I'll commit that first, since it will make things simpler for
> pg_dump.

Thanks for committing this one.

>> So to summarize what the patches do (some of these were posted earlier)
>>
>> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
> 
> I'm trying to understand why this is also different.  At least on an
> initial look, there seems to be a bunch more copy-and-paste from the
> existing TypedTable to Partition in gram.y, which seems to all boil down
> to removing the need for WITH OPTIONS to be specified.  If WITH OPTIONS
> would be too much to have included for partitioning, and it isn't
> actually necessary, why not just make it optional, and use the same
> construct for both, removing all the duplicaiton and the need for
> pg_dump to output it?

OK, I think optionally allowing WITH OPTIONS keywords would be nice.

So in lieu of this patch, I propose a patch that modifies gram.y to allow
WITH OPTIONS to specified.

>> 0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
>>   INHERIT to be emitted to attach a partition instead of ALTER TABLE
>>   ATTACH PARTITION
> 
> Well, worse, it was dumping out both, the INHERITs and the ATTACH
> PARTITION.

Oops, you're right.  Actually meant to say that.

> Given that we're now setting numParents for partitions, I
> would think we'd just move the if (tbinfo->partitionOf) branches up
> under the if (numParents > 0) branches, which I think would also help us
> catch additional issues, like the fact that a binary-upgrade with
> partitions in a different namespace from the partitioned table would
> fail because the ATTACH PARTITION code doesn't account for it:
> 
> appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
>   fmtId(tbinfo->partitionOf->dobj.name));
> appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
>   fmtId(tbinfo->dobj.name),
>   tbinfo->partitiondef);
> 
> unlike the existing inheritance code a few lines above, which does:
> 
> appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
>   fmtId(tbinfo->dobj.name));
> if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
> appendPQExpBuffer(q, "%s.",
> fmtId(parentRel->dobj.namespace->dobj.name));
> appendPQExpBuffer(q, "%s;\n",
>   fmtId(parentRel->dobj.name));

That's a good point.  I put both cases under the if (numParents > 0) block
and appropriate text is emitted depending on whether the table is a
partition or plain inheritance child.

>> 0004: Change the way pg_dump retrieves partitioning info (removes a lot
>>   of unnecessary code and instead makes partitioning case use the old
>>   inheritance code, etc.)
> 
> This looks pretty good, at least on first blush, probably in part
> because it's mostly removing code.  The CASE statement in the
> getTables() query isn't needed, once pg_get_partkeydef() has been
> changed to not throw an ERROR when passed a non-partitioned table OID.

Oh yes, fixed.

I put the patches in different order this time so that the refactoring
patch appears before the --binary-upgrade bug-fix patch (0003 and 0004
have reversed their roles.)  Also, 0002 is no longer a pg_dump fix.

0001: Improve test coverage of partition constraints (non-inherited ones)

0002: Allow partition columns to optionally include WITH OPTIONS keywords

0003: Change the way pg_dump retrieves partitioning info (removes a lot
  of unnecessary code and instead makes partitioning case use the old
  inheritance code, etc.)

0004: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
  INHERIT to be emitted to attach a partition in addition to of ALTER
  TABLE ATTACH PARTITION and that no schema-name was emitted where it
  should 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-26 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/04/26 0:42, Stephen Frost wrote:
> > I'm not sure what you mean here.  We're always going to call both
> > getInherits() and getPartitions() and run the queries in each, with the
> > way the code is written today.  In my experience working with pg_dump
> > and trying to not slow it down, the last thing we want to do is run two
> > independent queries when one will do.  Further, we aren't really
> > avoiding any work when we have to go look at pg_class to exclude the
> > partitioned tables in getInherits() anyway.  I'm not seeing why we
> > really need the join at all though, see below.
> 
> You're right and I realize that we don't need lots of new code for pg_dump
> to retrieve the partitioning information (including the parent-child
> relationships).  I propose some patches below, one per each item you
> discovered could be improved.

Thanks!

> Attached patch 0004 gets rid of that separation.  It removes the struct
> PartInfo, functions flagPartitions(), findPartitionParentByOid(),
> getPartitions(), and getTablePartitionKeyInfo().  All necessary
> partitioning-specific information is retrieved in getTables() itself.

That certainly looks better.

> Also, now that partitioning uses the old inheritance code, inherited NOT
> NULL constraints need not be emitted separately per child.  The
> now-removed code that separately retrieved partitioning inheritance
> information was implemented such that partition attributes didn't receive
> the flagInhAttrs() treatment.

Right.

> > I looked through
> > pg_get_partkeydef() and it didn't seem to be particularly expensive to
> > run, though evidently it doesn't handle being passed an OID that it
> > doesn't expect very cleanly:
> > 
> > =# select pg_get_partkeydef(oid) from pg_class;
> > ERROR:  cache lookup failed for partition key of 52333
> > 
> > I don't believe that's really very appropriate of it to do though and
> > instead it should work the same way pg_get_indexdef() and others do and
> > return NULL in such cases, so people can use it sanely.
> > 
> > I'm working through this but it's going to take a bit of time to clean
> > it up and it's not going to get finished today, but I do think it needs
> > to get done and so I'll work to make it happen this week.  I didn't
> > anticipate finding this, obviously and am a bit disappointed by it.
> 
> Yeah, that was sloppy. :-(
> 
> Attached patch 0005 fixes that and adds a test to rules.sql.

Good, I'll commit that first, since it will make things simpler for
pg_dump.

> I think I have found an oversight in the pg_dump's --binary-upgrade code
> that might have caused the error you saw (I proceeded to fix it after
> seeing the error that I saw).  Fix is included as patch 0003.

Yeah, that was what I saw also.

> So to summarize what the patches do (some of these were posted earlier)
> 
> 0001: Improve test coverage of partition constraints (non-inherited ones)

Looks fine.

> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns

I'm trying to understand why this is also different.  At least on an
initial look, there seems to be a bunch more copy-and-paste from the
existing TypedTable to Partition in gram.y, which seems to all boil down
to removing the need for WITH OPTIONS to be specified.  If WITH OPTIONS
would be too much to have included for partitioning, and it isn't
actually necessary, why not just make it optional, and use the same
construct for both, removing all the duplicaiton and the need for
pg_dump to output it?

> 0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
>   INHERIT to be emitted to attach a partition instead of ALTER TABLE
>   ATTACH PARTITION

Well, worse, it was dumping out both, the INHERITs and the ATTACH
PARTITION.  Given that we're now setting numParents for partitions, I
would think we'd just move the if (tbinfo->partitionOf) branches up
under the if (numParents > 0) branches, which I think would also help us
catch additional issues, like the fact that a binary-upgrade with
partitions in a different namespace from the partitioned table would
fail because the ATTACH PARTITION code doesn't account for it:

appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
  fmtId(tbinfo->partitionOf->dobj.name));
appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
  fmtId(tbinfo->dobj.name),
  tbinfo->partitiondef);

unlike the existing inheritance code a few lines above, which does:

appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
  fmtId(tbinfo->dobj.name));
if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
appendPQExpBuffer(q, "%s.",
fmtId(parentRel->dobj.namespace->dobj.name));
appendPQExpBuffer(q, 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-26 Thread Amit Langote
Hi Stephen,

On 2017/04/26 0:42, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> I think why getPartitions() is separate from getInherits() and then
>> flagPartitions() separate from flagInhTables() is because I thought
>> originally that mixing the two would be undesirable.  In the partitioning
>> case, getPartitions() joins pg_inherits with pg_class so that it can also
>> get hold of relpartbound, which I then thought would be a bad idea to do
>> for all of the inheritance tables.  If we're OK with always doing the
>> join, I don't see why we couldn't get rid of the separation.
> 
> I'm not sure what you mean here.  We're always going to call both
> getInherits() and getPartitions() and run the queries in each, with the
> way the code is written today.  In my experience working with pg_dump
> and trying to not slow it down, the last thing we want to do is run two
> independent queries when one will do.  Further, we aren't really
> avoiding any work when we have to go look at pg_class to exclude the
> partitioned tables in getInherits() anyway.  I'm not seeing why we
> really need the join at all though, see below.

You're right and I realize that we don't need lots of new code for pg_dump
to retrieve the partitioning information (including the parent-child
relationships).  I propose some patches below, one per each item you
discovered could be improved.

>> flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
>> it for both inheritance and partitioning is fine.  It affects NOT NULL
>> constraints and defaults, which as far as it goes, applies to both
>> inheritance and partitioning the same.
> 
> I don't see why we need to have getPartitions(), flagPartitions(), or
> findPartitonParentByOid().  They all seem to be largely copies of the
> inheritance functions, but it doesn't seem like that's really necessary
> or sensible.
> 
> I also don't follow why we're pulling the partbound definition in
> getPartitions() instead of in getTables(), where we're already pulling
> the rest of the data from pg_class?  Or why getTablePartitionKeyInfo()
> exists instead of also doing that during getTables()?

I think it had to do with wanting to avoid creating yet another copy of
the big getTables() query for >= v10, back when the original patch was
written, but doing that is not really needed.

Attached patch 0004 gets rid of that separation.  It removes the struct
PartInfo, functions flagPartitions(), findPartitionParentByOid(),
getPartitions(), and getTablePartitionKeyInfo().  All necessary
partitioning-specific information is retrieved in getTables() itself.

Also, now that partitioning uses the old inheritance code, inherited NOT
NULL constraints need not be emitted separately per child.  The
now-removed code that separately retrieved partitioning inheritance
information was implemented such that partition attributes didn't receive
the flagInhAttrs() treatment.

> I looked through
> pg_get_partkeydef() and it didn't seem to be particularly expensive to
> run, though evidently it doesn't handle being passed an OID that it
> doesn't expect very cleanly:
> 
> =# select pg_get_partkeydef(oid) from pg_class;
> ERROR:  cache lookup failed for partition key of 52333
> 
> I don't believe that's really very appropriate of it to do though and
> instead it should work the same way pg_get_indexdef() and others do and
> return NULL in such cases, so people can use it sanely.
> 
> I'm working through this but it's going to take a bit of time to clean
> it up and it's not going to get finished today, but I do think it needs
> to get done and so I'll work to make it happen this week.  I didn't
> anticipate finding this, obviously and am a bit disappointed by it.

Yeah, that was sloppy. :-(

Attached patch 0005 fixes that and adds a test to rules.sql.

>> So, the newly added tests seem enough for now?
> 
> Considering the pg_upgrade regression test didn't pass with the patch
> as sent, I'd say that more tests are needed.  I will be working to add
> those also.

I think I have found an oversight in the pg_dump's --binary-upgrade code
that might have caused the error you saw (I proceeded to fix it after
seeing the error that I saw).  Fix is included as patch 0003.

So to summarize what the patches do (some of these were posted earlier)

0001: Improve test coverage of partition constraints (non-inherited ones)

0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns

0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
  INHERIT to be emitted to attach a partition instead of ALTER TABLE
  ATTACH PARTITION

0004: Change the way pg_dump retrieves partitioning info (removes a lot
  of unnecessary code and instead makes partitioning case use the old
  inheritance code, etc.)

0005: Teach pg_get_partkeydef_worker to deal with OIDs it can't handle

Thanks,
Amit
>From 5ba1b532b020648457e92d34d5f2712f36efd9ff Mon Sep 17 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-25 Thread Robert Haas
On Tue, Apr 25, 2017 at 12:26 PM, Stephen Frost  wrote:
> Interesting.  Seems like the question is really what we mean by "ONLY"
> here.  For my 2c, at least, if we can check that all of the partitions
> already have the constraint enforced, such that the only thing we're
> changing is the partitioned table, then that's exactly what "ONLY"
> should do.  That would require a bit of rework, presumably, of how that
> keyword is handled but the basics are all there.

Well, I'm not saying that couldn't be done, but it doesn't add any
capability that we don't have today, so from my point of view it seems
fairly pointless.  However, we can leave that for another day; for
now, I think we should focus on fixing the pg_dump bugs that this
thread is about.

> I'm planning to push just this patch to make the backend accept the
> ALTER TABLE ONLY when no partitions exist later this afternoon, ...

Cool.

> but the
> work here isn't done as noted in my other email.

Right.  I'd just like to get the work done as soon as we reasonably
can.  Tempus fugit, and all that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-25 Thread Stephen Frost
Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Apr 24, 2017 at 9:17 AM, Stephen Frost  wrote:
> > I wonder why the restriction is there, which is probably part of the
> > reason that I'm thinking of phrasing the documentation that way.
> >
> > Beyond a matter of round to-its, is there a reason why it couldn't (or
> > shouldn't) be supported?  I'm not suggesting now, of course, but in a
> > future release.
> 
> I don't see any particular reason, but I haven't looked into the
> matter deeply.  One thing to consider is that you can already more or
> less do exactly that thing, like this:
> 
> rhaas=# create table foo (a int, b text) partition by range (a);
> CREATE TABLE
> Time: 4.738 ms
> rhaas=# create table foo1 partition of foo for values from (0) to (100);
> CREATE TABLE
> Time: 5.162 ms
> rhaas=# insert into foo1 select 1, 'snarf' from generate_series(1,1000) g;
> INSERT 0 1000
> Time: 12835.153 ms (00:12.835)
> rhaas=# alter table foo1 add constraint xyz check (b != 'nope');
> ALTER TABLE
> Time: 1059.728 ms (00:01.060)
> rhaas=# alter table foo add constraint xyz check (b != 'nope');
> NOTICE:  merging constraint "xyz" with inherited definition
> ALTER TABLE
> Time: 1.243 ms
> 
> So we may not really need another way to do it.

Interesting.  Seems like the question is really what we mean by "ONLY"
here.  For my 2c, at least, if we can check that all of the partitions
already have the constraint enforced, such that the only thing we're
changing is the partitioned table, then that's exactly what "ONLY"
should do.  That would require a bit of rework, presumably, of how that
keyword is handled but the basics are all there.

> >> Also, I think saying that it it will result in an error is a bit more
> >> helpful than saying that it is is not supported, since the latter
> >> might lead someone to believe that it could result in undefined
> >> behavior (i.e. arbitrarily awful misbehavior) which is not the case.
> >> So I like what he wrote, for whatever that's worth.
> >
> > I don't mind stating that it'll result in an error, I just don't want to
> > imply that there's some way to get around that error if things were done
> > differently.
> >
> > How about:
> >
> > ---
> > Once partitions exist, using ONLY will always result
> > in an error as adding or dropping constraints on only the partitiioned
> > table, when partitions exist, is not supported.
> > ---
> 
> I still prefer Amit's language, but it's not worth to me to keep
> arguing about it.  If you prefer what you've written there, fine, but
> let's get something committed and move on.  I think we understand what
> needs to be fixed here now, and I'd like to do get that done.  If you
> don't want to do it or don't have time to do it, I'll pick it up
> again, but let's not let this keep dragging out.

I'm planning to push just this patch to make the backend accept the
ALTER TABLE ONLY when no partitions exist later this afternoon, but the
work here isn't done as noted in my other email.

Thanks!

Stephen
From c51fbaabf4da121c8bfd68b7bf74db1fbea4e581 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Mon, 17 Apr 2017 12:06:12 -0400
Subject: [PATCH] Allow ALTER TABLE ONLY on partitioned tables

There is no need to forbid ALTER TABLE ONLY on partitioned tables,
when no partitions exist yet.  This can be handy for users who are
building up their partitioned table independently and will create actual
partitions later.

In addition, this is how pg_dump likes to operate in certain instances.

Author: Amit Langote, with some error message word-smithing by me
---
 doc/src/sgml/ddl.sgml | 18 ++---
 src/backend/commands/tablecmds.c  | 66 +++
 src/test/regress/expected/alter_table.out | 36 +++--
 src/test/regress/expected/truncate.out|  8 
 src/test/regress/sql/alter_table.sql  | 24 ---
 src/test/regress/sql/truncate.sql |  4 ++
 6 files changed, 108 insertions(+), 48 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 340c961..84c4f20 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2944,17 +2944,23 @@ VALUES ('Albany', NULL, NULL, 'NY');
Both CHECK and NOT NULL
constraints of a partitioned table are always inherited by all its
partitions.  CHECK constraints that are marked
-   NO INHERIT are not allowed.
+   NO INHERIT are not allowed to be created on
+   partitioned tables.
   
  
 
  
   
-   The ONLY notation used to exclude child tables
-   will cause an error for partitioned tables in the case of
-   schema-modifying commands such as most ALTER TABLE
-   commands.  For example, dropping a column from only the parent does
-   not make sense for partitioned tables.
+   Using ONLY to add or drop a constraint on only the
+   partitioned table is supported when there are no 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-25 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> I think why getPartitions() is separate from getInherits() and then
> flagPartitions() separate from flagInhTables() is because I thought
> originally that mixing the two would be undesirable.  In the partitioning
> case, getPartitions() joins pg_inherits with pg_class so that it can also
> get hold of relpartbound, which I then thought would be a bad idea to do
> for all of the inheritance tables.  If we're OK with always doing the
> join, I don't see why we couldn't get rid of the separation.

I'm not sure what you mean here.  We're always going to call both
getInherits() and getPartitions() and run the queries in each, with the
way the code is written today.  In my experience working with pg_dump
and trying to not slow it down, the last thing we want to do is run two
independent queries when one will do.  Further, we aren't really
avoiding any work when we have to go look at pg_class to exclude the
partitioned tables in getInherits() anyway.  I'm not seeing why we
really need the join at all though, see below.

> flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
> it for both inheritance and partitioning is fine.  It affects NOT NULL
> constraints and defaults, which as far as it goes, applies to both
> inheritance and partitioning the same.

I don't see why we need to have getPartitions(), flagPartitions(), or
findPartitonParentByOid().  They all seem to be largely copies of the
inheritance functions, but it doesn't seem like that's really necessary
or sensible.

I also don't follow why we're pulling the partbound definition in
getPartitions() instead of in getTables(), where we're already pulling
the rest of the data from pg_class?  Or why getTablePartitionKeyInfo()
exists instead of also doing that during getTables()?  I looked through
pg_get_partkeydef() and it didn't seem to be particularly expensive to
run, though evidently it doesn't handle being passed an OID that it
doesn't expect very cleanly:

=# select pg_get_partkeydef(oid) from pg_class;
ERROR:  cache lookup failed for partition key of 52333

I don't believe that's really very appropriate of it to do though and
instead it should work the same way pg_get_indexdef() and others do and
return NULL in such cases, so people can use it sanely.

I'm working through this but it's going to take a bit of time to clean
it up and it's not going to get finished today, but I do think it needs
to get done and so I'll work to make it happen this week.  I didn't
anticipate finding this, obviously and am a bit disappointed by it.

> So, the newly added tests seem enough for now?

Considering the pg_upgrade regression test didn't pass with the patch
as sent, I'd say that more tests are needed.  I will be working to add
those also.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-24 Thread Robert Haas
On Mon, Apr 24, 2017 at 9:17 AM, Stephen Frost  wrote:
> I wonder why the restriction is there, which is probably part of the
> reason that I'm thinking of phrasing the documentation that way.
>
> Beyond a matter of round to-its, is there a reason why it couldn't (or
> shouldn't) be supported?  I'm not suggesting now, of course, but in a
> future release.

I don't see any particular reason, but I haven't looked into the
matter deeply.  One thing to consider is that you can already more or
less do exactly that thing, like this:

rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
Time: 4.738 ms
rhaas=# create table foo1 partition of foo for values from (0) to (100);
CREATE TABLE
Time: 5.162 ms
rhaas=# insert into foo1 select 1, 'snarf' from generate_series(1,1000) g;
INSERT 0 1000
Time: 12835.153 ms (00:12.835)
rhaas=# alter table foo1 add constraint xyz check (b != 'nope');
ALTER TABLE
Time: 1059.728 ms (00:01.060)
rhaas=# alter table foo add constraint xyz check (b != 'nope');
NOTICE:  merging constraint "xyz" with inherited definition
ALTER TABLE
Time: 1.243 ms

So we may not really need another way to do it.

>> Also, I think saying that it it will result in an error is a bit more
>> helpful than saying that it is is not supported, since the latter
>> might lead someone to believe that it could result in undefined
>> behavior (i.e. arbitrarily awful misbehavior) which is not the case.
>> So I like what he wrote, for whatever that's worth.
>
> I don't mind stating that it'll result in an error, I just don't want to
> imply that there's some way to get around that error if things were done
> differently.
>
> How about:
>
> ---
> Once partitions exist, using ONLY will always result
> in an error as adding or dropping constraints on only the partitiioned
> table, when partitions exist, is not supported.
> ---

I still prefer Amit's language, but it's not worth to me to keep
arguing about it.  If you prefer what you've written there, fine, but
let's get something committed and move on.  I think we understand what
needs to be fixed here now, and I'd like to do get that done.  If you
don't want to do it or don't have time to do it, I'll pick it up
again, but let's not let this keep dragging out.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-24 Thread Stephen Frost
Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 21, 2017 at 1:43 AM, Stephen Frost  wrote:
> >> + Once
> >> +   partitions exist, we do not support using ONLY 
> >> to
> >> +   add or drop constraints on only the partitioned table.
> >>
> >> I wonder if the following sounds a bit more informative: Once partitions
> >> exist, using ONLY will result in an error, because the
> >> constraint being added or dropped must be added or dropped from the
> >> partitions too.
> >
> > My thinking here is that we may add support later on down the line for
> > using the ONLY keyword, when the constraint has already been created on
> > the partitions themselves, so I would rather just clarify that we don't
> > (yet) support that.  Your wording, at least to me, seems to imply that
> > if the constraint was added to or dropped from the partitions then the
> > ONLY keyword could be used, which isn't accurate today.
> 
> Well, I think that Amit's wording has the virtue of giving a reason
> which is basically valid, whereas someone reading your text might
> conceivably be left wondering what the reason for the restriction is.

I wonder why the restriction is there, which is probably part of the
reason that I'm thinking of phrasing the documentation that way.

Beyond a matter of round to-its, is there a reason why it couldn't (or
shouldn't) be supported?  I'm not suggesting now, of course, but in a
future release.

I could certainly see someone wanting to manage the process by which a
constriant is added by adding it one at a time to the individual
partitions and then wishing to be sure that adding it to parent only
checked that the individual partitions had the constraint and then added
it to the parent (in other words, a relatively short operation,
providing the locks are able to be acquired quickly).

> Also, I think saying that it it will result in an error is a bit more
> helpful than saying that it is is not supported, since the latter
> might lead someone to believe that it could result in undefined
> behavior (i.e. arbitrarily awful misbehavior) which is not the case.
> So I like what he wrote, for whatever that's worth.

I don't mind stating that it'll result in an error, I just don't want to
imply that there's some way to get around that error if things were done
differently.

How about:

---
Once partitions exist, using ONLY will always result
in an error as adding or dropping constraints on only the partitiioned
table, when partitions exist, is not supported.
---

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-23 Thread Stephen Frost
Noah, all,

On Sun, Apr 23, 2017 at 19:52 Noah Misch  wrote:

> On Sat, Apr 22, 2017 at 01:14:08PM -0700, Noah Misch wrote:
> > On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote:
> > > * Noah Misch (n...@leadboat.com) wrote:
> > > > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote:
> > > > > I've put up a new patch for review on the thread and plan to commit
> > > > > that tomorrow, assuming there isn't anything further.  That should
> > > > > resolve the immediate issue, but I do think there's some merit to
> Amit's
> > > > > follow-on patches and will continue to discuss those and may
> commit one
> > > > > or both of those later this week.
> > > >
> > > > This PostgreSQL 10 open item is past due for your status update.
> Kindly send
> > > > a status update within 24 hours, and include a date for your
> subsequent status
> > > > update.  Refer to the policy on open item ownership:
> > > >
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> > >
> > > I thought I'd be able to get to this today, but other activities have
> > > taken up the time I had planned to work on this.  I'll be on it again
> > > tomorrow morning and will provide an update (most likely a couple of
> > > commits) by COB tomorrow.
> >
> > This PostgreSQL 10 open item is again past due for your status update.
> Kindly
> > send a status update within 24 hours, and include a date for your
> subsequent
> > status update.  Refer to the policy on open item ownership:
> >
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>
> IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past
> due
> for your status update.  Please reacquaint yourself with the policy on open
> item ownership[1] and then reply immediately.  If I do not hear from you by
> 2017-04-25 00:00 UTC, I will transfer this item to release management team
> ownership without further notice.


The status is simply that I've been considering Robert's comments regarding
the documentation and have had a busy weekend. I'll provide an update
tomorrow.

Thanks!

Stephen

>


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-23 Thread Noah Misch
On Sat, Apr 22, 2017 at 01:14:08PM -0700, Noah Misch wrote:
> On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote:
> > * Noah Misch (n...@leadboat.com) wrote:
> > > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote:
> > > > I've put up a new patch for review on the thread and plan to commit
> > > > that tomorrow, assuming there isn't anything further.  That should
> > > > resolve the immediate issue, but I do think there's some merit to Amit's
> > > > follow-on patches and will continue to discuss those and may commit one
> > > > or both of those later this week.
> > > 
> > > This PostgreSQL 10 open item is past due for your status update.  Kindly 
> > > send
> > > a status update within 24 hours, and include a date for your subsequent 
> > > status
> > > update.  Refer to the policy on open item ownership:
> > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> > 
> > I thought I'd be able to get to this today, but other activities have
> > taken up the time I had planned to work on this.  I'll be on it again
> > tomorrow morning and will provide an update (most likely a couple of
> > commits) by COB tomorrow.
> 
> This PostgreSQL 10 open item is again past due for your status update.  Kindly
> send a status update within 24 hours, and include a date for your subsequent
> status update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
for your status update.  Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately.  If I do not hear from you by
2017-04-25 00:00 UTC, I will transfer this item to release management team
ownership without further notice.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-22 Thread Noah Misch
On Thu, Apr 20, 2017 at 09:53:28PM -0400, Stephen Frost wrote:
> * Noah Misch (n...@leadboat.com) wrote:
> > On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote:
> > > I've put up a new patch for review on the thread and plan to commit
> > > that tomorrow, assuming there isn't anything further.  That should
> > > resolve the immediate issue, but I do think there's some merit to Amit's
> > > follow-on patches and will continue to discuss those and may commit one
> > > or both of those later this week.
> > 
> > This PostgreSQL 10 open item is past due for your status update.  Kindly 
> > send
> > a status update within 24 hours, and include a date for your subsequent 
> > status
> > update.  Refer to the policy on open item ownership:
> > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> I thought I'd be able to get to this today, but other activities have
> taken up the time I had planned to work on this.  I'll be on it again
> tomorrow morning and will provide an update (most likely a couple of
> commits) by COB tomorrow.

This PostgreSQL 10 open item is again past due for your status update.  Kindly
send a status update within 24 hours, and include a date for your subsequent
status update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-21 Thread Robert Haas
On Fri, Apr 21, 2017 at 1:43 AM, Stephen Frost  wrote:
>> + Once
>> +   partitions exist, we do not support using ONLY to
>> +   add or drop constraints on only the partitioned table.
>>
>> I wonder if the following sounds a bit more informative: Once partitions
>> exist, using ONLY will result in an error, because the
>> constraint being added or dropped must be added or dropped from the
>> partitions too.
>
> My thinking here is that we may add support later on down the line for
> using the ONLY keyword, when the constraint has already been created on
> the partitions themselves, so I would rather just clarify that we don't
> (yet) support that.  Your wording, at least to me, seems to imply that
> if the constraint was added to or dropped from the partitions then the
> ONLY keyword could be used, which isn't accurate today.

Well, I think that Amit's wording has the virtue of giving a reason
which is basically valid, whereas someone reading your text might
conceivably be left wondering what the reason for the restriction is.
Also, I think saying that it it will result in an error is a bit more
helpful than saying that it is is not supported, since the latter
might lead someone to believe that it could result in undefined
behavior (i.e. arbitrarily awful misbehavior) which is not the case.
So I like what he wrote, for whatever that's worth.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-20 Thread Amit Langote
Hi Stephen,

On 2017/04/21 8:43, Stephen Frost wrote:
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> On 2017/04/18 1:43, Stephen Frost wrote:
>>> Please take a look at the attached and let me know your thoughts on it.
>>> I changed the code to complain again regarding TRUNCATE ONLY, since that
>>> never actually makes sense on a partitioned table, unlike ALTER TABLE
>>> ONLY, and adjusted the error messages and added hints.
>>
>> Thanks for polishing the patch.  It looks mostly good to me.
>>
>> + Once
>> +   partitions exist, we do not support using ONLY to
>> +   add or drop constraints on only the partitioned table.
>>
>> I wonder if the following sounds a bit more informative: Once partitions
>> exist, using ONLY will result in an error, because the
>> constraint being added or dropped must be added or dropped from the
>> partitions too.
> 
> My thinking here is that we may add support later on down the line for
> using the ONLY keyword, when the constraint has already been created on
> the partitions themselves, so I would rather just clarify that we don't
> (yet) support that.

OK, I wanted to word it like that, because I have been thinking that we
will never support that.

So right now, we do not support ONLY (or how I tend to read it: cause
error on specifying ONLY) if partitions exist at all.  In the future, we
will not output error until we have also seen that some partition does not
possess the constraint being added.  IOW, specifying ONLY won't cause an
error even with non-zero partitions if all of them have the constraint
already defined.  Maybe that's something we will support, but I am not
sure how many users will want to do things that way instead of the other
way around; I mean if the constraint is to be enforced on the whole
partitioned table anyway, why not just define it on the partitioned table
in the first place.  But then again, why restrict users in many number of
ways.

So alright, I'm fine with the wording.  If someone complains later that
they want more clarification on that point, it could be done later.

> Your wording, at least to me, seems to imply that
> if the constraint was added to or dropped from the partitions then the
> ONLY keyword could be used, which isn't accurate today.

Yes, that's likely to be the impression.

 Updated patches attached (0002 and 0003 unchanged).
>>>
>>> Regarding these, 0002 changes pg_dump to handle partitions much more
>>> like inheritance, which at least seems like a decent idea but makes me a
>>> bit concerned about making sure we're doing everything correctly.
>>
>> Are you concerned about the correctness of the code in pg_dump or backend?
> 
> My concern is with regard to pg_dump in this case, primairly.
> 
>> Rules of inheritance enforced by flagInhAttrs() are equally applicable to
>> the partitioning case, so actions performed by it for the partitioning
>> cases will not emit incorrect text.
> 
> I understand that this *should* be the case, but that's not how the code
> in pg_dump was written originally when it came to native partitions,
> having its own flagPartitions() function in fact, which makes me
> hesitate to start assuming what we do for inheritance is going to work
> in these cases the same.

I think why getPartitions() is separate from getInherits() and then
flagPartitions() separate from flagInhTables() is because I thought
originally that mixing the two would be undesirable.  In the partitioning
case, getPartitions() joins pg_inherits with pg_class so that it can also
get hold of relpartbound, which I then thought would be a bad idea to do
for all of the inheritance tables.  If we're OK with always doing the
join, I don't see why we couldn't get rid of the separation.

flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
it for both inheritance and partitioning is fine.  It affects NOT NULL
constraints and defaults, which as far as it goes, applies to both
inheritance and partitioning the same.

>>> In
>>> particular, we should really have regression tests for non-inherited
>>> CHECK (and NOT NULL) constraints on partitions.  Perhaps there are tests
>>> covering those cases in the standard regression suite, but it's not
>>> obvious from the SQL.
>>
>> There is one in create_table.sql that looks like this:
>>
>> CREATE TABLE part_b PARTITION OF parted (
>> b NOT NULL DEFAULT 1 CHECK (b >= 0),
>> CONSTRAINT check_a CHECK (length(a) > 0)
>> ) FOR VALUES IN ('b');
>> -- conislocal should be false for any merged constraints
>> SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
>> 'part_b'::regclass AND conname = 'check_a';
>>
>> But it doesn't really look like it's testing non-inherited constraints a
>> whole lot (CHECK (b >= 0) above is a local non-inherited constraint).
>>
>> I added a few more tests right below the above test so that there is a bit
>> more coverage.  Keep the rule I mentioned above when looking at these.  I
>> also added a test in the pg_dump 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-20 Thread Stephen Frost
Greetings,

* Noah Misch (n...@leadboat.com) wrote:
> On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote:
> > I've put up a new patch for review on the thread and plan to commit
> > that tomorrow, assuming there isn't anything further.  That should
> > resolve the immediate issue, but I do think there's some merit to Amit's
> > follow-on patches and will continue to discuss those and may commit one
> > or both of those later this week.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

I thought I'd be able to get to this today, but other activities have
taken up the time I had planned to work on this.  I'll be on it again
tomorrow morning and will provide an update (most likely a couple of
commits) by COB tomorrow.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-20 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/04/18 1:43, Stephen Frost wrote:
> > * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> >> OK, I agree.  I tweaked the existing bullet point about differences from
> >> traditional inheritance when using ONLY with partitioned tables.
> > 
> > Please take a look at the attached and let me know your thoughts on it.
> > I changed the code to complain again regarding TRUNCATE ONLY, since that
> > never actually makes sense on a partitioned table, unlike ALTER TABLE
> > ONLY, and adjusted the error messages and added hints.
> 
> Thanks for polishing the patch.  It looks mostly good to me.
> 
> + Once
> +   partitions exist, we do not support using ONLY to
> +   add or drop constraints on only the partitioned table.
> 
> I wonder if the following sounds a bit more informative: Once partitions
> exist, using ONLY will result in an error, because the
> constraint being added or dropped must be added or dropped from the
> partitions too.

My thinking here is that we may add support later on down the line for
using the ONLY keyword, when the constraint has already been created on
the partitions themselves, so I would rather just clarify that we don't
(yet) support that.  Your wording, at least to me, seems to imply that
if the constraint was added to or dropped from the partitions then the
ONLY keyword could be used, which isn't accurate today.

> >> Updated patches attached (0002 and 0003 unchanged).
> > 
> > Regarding these, 0002 changes pg_dump to handle partitions much more
> > like inheritance, which at least seems like a decent idea but makes me a
> > bit concerned about making sure we're doing everything correctly.
> 
> Are you concerned about the correctness of the code in pg_dump or backend?

My concern is with regard to pg_dump in this case, primairly.

> Rules of inheritance enforced by flagInhAttrs() are equally applicable to
> the partitioning case, so actions performed by it for the partitioning
> cases will not emit incorrect text.

I understand that this *should* be the case, but that's not how the code
in pg_dump was written originally when it came to native partitions,
having its own flagPartitions() function in fact, which makes me
hesitate to start assuming what we do for inheritance is going to work
in these cases the same.

> The rule that backend follows is this: if a constraint is added to the
> partitioned table (either a named check constraint or a NOT NULL
> constraint), that constraint becomes an inherited *non-local* constraint
> in all the partitions no matter if it was present in the partitions before
> or not.

Right, I get that.

> > In
> > particular, we should really have regression tests for non-inherited
> > CHECK (and NOT NULL) constraints on partitions.  Perhaps there are tests
> > covering those cases in the standard regression suite, but it's not
> > obvious from the SQL.
> 
> There is one in create_table.sql that looks like this:
> 
> CREATE TABLE part_b PARTITION OF parted (
> b NOT NULL DEFAULT 1 CHECK (b >= 0),
> CONSTRAINT check_a CHECK (length(a) > 0)
> ) FOR VALUES IN ('b');
> -- conislocal should be false for any merged constraints
> SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
> 'part_b'::regclass AND conname = 'check_a';
> 
> But it doesn't really look like it's testing non-inherited constraints a
> whole lot (CHECK (b >= 0) above is a local non-inherited constraint).
> 
> I added a few more tests right below the above test so that there is a bit
> more coverage.  Keep the rule I mentioned above when looking at these.  I
> also added a test in the pg_dump suite.  That's in patch 0001 (since you
> posted your version of what was 0001 before, I'm no longer including it here).

Right, I saw the additional tests in your original patch, though it
DROP'd at least parted_no_parts, meaning that it wasn't being tested as
part of pg_upgrade/pg_dump testing.

> By the way, 0003 is a bug-fix.  WITH OPTIONS that is emitted currently
> like below is not the acceptable syntax:
> 
> CREATE TABLE a_partition OF a_partitioned_table (
> a WITH OPTIONS NOT NULL DEFAULT 1
> ) FOR VALUES blah
> 
> But looking at the pg_dump code closely and also considering our
> discussion upthread, I don't think this form is ever emitted.  Because we
> never emit a partition's column-level constraints and column defaults in
> the CREATE TABLE, but rather separately using ALTER TABLE.  In any case,
> applying 0003 seems to be the right thing to do anyway, in case we might
> rejigger things so that whatever can be emitted within the CREATE TABLE
> text will be.

Hm, ok.

I'm going over your latest set of patches.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-19 Thread Noah Misch
On Mon, Apr 17, 2017 at 03:41:25PM -0400, Stephen Frost wrote:
> * Noah Misch (n...@leadboat.com) wrote:
> > On Thu, Apr 13, 2017 at 11:38:08AM -0400, Robert Haas wrote:
> > > On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost  
> > > wrote:
> > > > Sure, though I won't be able to today and I've got some doubts about the
> > > > other patches.  I'll have more time tomorrow though.
> > > 
> > > OK, cool.  I'll mark you down as the owner on the open items list.
> > 
> > [Action required within three days.]
> 
> I've put up a new patch for review on the thread and plan to commit
> that tomorrow, assuming there isn't anything further.  That should
> resolve the immediate issue, but I do think there's some merit to Amit's
> follow-on patches and will continue to discuss those and may commit one
> or both of those later this week.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-17 Thread Amit Langote
Hi Stephen,

On 2017/04/18 1:43, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> OK, I agree.  I tweaked the existing bullet point about differences from
>> traditional inheritance when using ONLY with partitioned tables.
> 
> Please take a look at the attached and let me know your thoughts on it.
> I changed the code to complain again regarding TRUNCATE ONLY, since that
> never actually makes sense on a partitioned table, unlike ALTER TABLE
> ONLY, and adjusted the error messages and added hints.

Thanks for polishing the patch.  It looks mostly good to me.

+ Once
+   partitions exist, we do not support using ONLY to
+   add or drop constraints on only the partitioned table.

I wonder if the following sounds a bit more informative: Once partitions
exist, using ONLY will result in an error, because the
constraint being added or dropped must be added or dropped from the
partitions too.

>> Updated patches attached (0002 and 0003 unchanged).
> 
> Regarding these, 0002 changes pg_dump to handle partitions much more
> like inheritance, which at least seems like a decent idea but makes me a
> bit concerned about making sure we're doing everything correctly.

Are you concerned about the correctness of the code in pg_dump or backend?

Rules of inheritance enforced by flagInhAttrs() are equally applicable to
the partitioning case, so actions performed by it for the partitioning
cases will not emit incorrect text.

The rule that backend follows is this: if a constraint is added to the
partitioned table (either a named check constraint or a NOT NULL
constraint), that constraint becomes an inherited *non-local* constraint
in all the partitions no matter if it was present in the partitions before
or not.

> In
> particular, we should really have regression tests for non-inherited
> CHECK (and NOT NULL) constraints on partitions.  Perhaps there are tests
> covering those cases in the standard regression suite, but it's not
> obvious from the SQL.

There is one in create_table.sql that looks like this:

CREATE TABLE part_b PARTITION OF parted (
b NOT NULL DEFAULT 1 CHECK (b >= 0),
CONSTRAINT check_a CHECK (length(a) > 0)
) FOR VALUES IN ('b');
-- conislocal should be false for any merged constraints
SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
'part_b'::regclass AND conname = 'check_a';

But it doesn't really look like it's testing non-inherited constraints a
whole lot (CHECK (b >= 0) above is a local non-inherited constraint).

I added a few more tests right below the above test so that there is a bit
more coverage.  Keep the rule I mentioned above when looking at these.  I
also added a test in the pg_dump suite.  That's in patch 0001 (since you
posted your version of what was 0001 before, I'm no longer including it here).

By the way, 0003 is a bug-fix.  WITH OPTIONS that is emitted currently
like below is not the acceptable syntax:

CREATE TABLE a_partition OF a_partitioned_table (
a WITH OPTIONS NOT NULL DEFAULT 1
) FOR VALUES blah

But looking at the pg_dump code closely and also considering our
discussion upthread, I don't think this form is ever emitted.  Because we
never emit a partition's column-level constraints and column defaults in
the CREATE TABLE, but rather separately using ALTER TABLE.  In any case,
applying 0003 seems to be the right thing to do anyway, in case we might
rejigger things so that whatever can be emitted within the CREATE TABLE
text will be.

Thanks,
Amit
>From eba30bebbd29f79af778c9526e2d205e721f38c0 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 18 Apr 2017 10:59:35 +0900
Subject: [PATCH 1/3] Improve test coverage of partition constraints

Better exercise the code manipulating partition constraints a bit
differently from the traditional inheritance children.
---
 src/bin/pg_dump/t/002_pg_dump.pl   | 10 +---
 src/test/regress/expected/create_table.out | 41 ++
 src/test/regress/sql/create_table.sql  | 21 ---
 3 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ccd0ed6a53..bebb2f4f5d 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4757,12 +4757,16 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 		catch_all=> 'CREATE ... commands',
 		create_order => 91,
 		create_sql => 'CREATE TABLE dump_test_second_schema.measurement_y2006m2
-	   PARTITION OF dump_test.measurement FOR VALUES
-	   FROM (\'2006-02-01\') TO (\'2006-03-01\');',
+	   PARTITION OF dump_test.measurement (
+	  unitsales DEFAULT 0 CHECK (unitsales >= 0)
+	   )
+	   FOR VALUES FROM (\'2006-02-01\') TO (\'2006-03-01\');',
 		regexp => qr/^
 			\Q-- Name: measurement_y2006m2;\E.*\n
 			\Q--\E\n\n
-			\QCREATE TABLE measurement_y2006m2 PARTITION OF dump_test.measurement\E\n
+	

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-17 Thread Stephen Frost
Noah, all,

* Noah Misch (n...@leadboat.com) wrote:
> On Thu, Apr 13, 2017 at 11:38:08AM -0400, Robert Haas wrote:
> > On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost  wrote:
> > > Sure, though I won't be able to today and I've got some doubts about the
> > > other patches.  I'll have more time tomorrow though.
> > 
> > OK, cool.  I'll mark you down as the owner on the open items list.
> 
> [Action required within three days.]

I've put up a new patch for review on the thread and plan to commit
that tomorrow, assuming there isn't anything further.  That should
resolve the immediate issue, but I do think there's some merit to Amit's
follow-on patches and will continue to discuss those and may commit one
or both of those later this week.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-17 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> OK, I agree.  I tweaked the existing bullet point about differences from
> traditional inheritance when using ONLY with partitioned tables.

Please take a look at the attached and let me know your thoughts on it.
I changed the code to complain again regarding TRUNCATE ONLY, since that
never actually makes sense on a partitioned table, unlike ALTER TABLE
ONLY, and adjusted the error messages and added hints.

> Updated patches attached (0002 and 0003 unchanged).

Regarding these, 0002 changes pg_dump to handle partitions much more
like inheritance, which at least seems like a decent idea but makes me a
bit concerned about making sure we're doing everything correctly.  In
particular, we should really have regression tests for non-inherited
CHECK (and NOT NULL) constraints on partitions.  Perhaps there are tests
covering those cases in the standard regression suite, but it's not
obvious from the SQL.

Thanks!

Stephen
From ecea1d99bb1bce1e1625d4f0157af03274c82e17 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Mon, 17 Apr 2017 12:06:12 -0400
Subject: [PATCH] Allow ALTER TABLE ONLY on partitioned tables

There is no need to forbid ALTER TABLE ONLY on partitioned tables,
when no partitions exist yet.  This can be handy for users who are
building up their partitioned table independently and will create actual
partitions later.

In addition, this is how pg_dump likes to operate in certain instances.

Author: Amit Langote, with some error message word-smithing by me
---
 doc/src/sgml/ddl.sgml | 17 +---
 src/backend/commands/tablecmds.c  | 66 +++
 src/test/regress/expected/alter_table.out | 36 +++--
 src/test/regress/expected/truncate.out|  8 
 src/test/regress/sql/alter_table.sql  | 24 ---
 src/test/regress/sql/truncate.sql |  4 ++
 6 files changed, 107 insertions(+), 48 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 340c961..958a90a 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2944,17 +2944,22 @@ VALUES ('Albany', NULL, NULL, 'NY');
Both CHECK and NOT NULL
constraints of a partitioned table are always inherited by all its
partitions.  CHECK constraints that are marked
-   NO INHERIT are not allowed.
+   NO INHERIT are not allowed to be created on
+   partitioned tables.
   
  
 
  
   
-   The ONLY notation used to exclude child tables
-   will cause an error for partitioned tables in the case of
-   schema-modifying commands such as most ALTER TABLE
-   commands.  For example, dropping a column from only the parent does
-   not make sense for partitioned tables.
+   Using ONLY to add or drop a constraint on only the
+   partitioned table is only supported when no partitions exist yet.  Once
+   partitions exist, we do not support using ONLY to
+   add or drop constraints on only the partitioned table.  Instead,
+   constraints can be added or dropped, when they are not present in the
+   parent table, directly on the partitions.  As a partitioned table does
+   not have any data directly, attempts to use TRUNCATE
+   ONLY on a partitioned table will always return an
+   error.
   
  
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a02904c..a357130 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1259,7 +1259,8 @@ ExecuteTruncate(TruncateStmt *stmt)
 		else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 			ereport(ERROR,
 	(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-	 errmsg("must truncate child tables too")));
+	 errmsg("cannot truncate only a partitioned table"),
+	 errhint("Do not specify the ONLY keyword, or use truncate only on the partitions directly.")));
 	}
 
 	/*
@@ -5578,14 +5579,20 @@ static void
 ATPrepDropNotNull(Relation rel, bool recurse, bool recursing)
 {
 	/*
-	 * If the parent is a partitioned table, like check constraints, NOT NULL
-	 * constraints must be dropped from child tables.
+	 * If the parent is a partitioned table, like check constraints, we do
+	 * not support removing the NOT NULL while partitions exist.
 	 */
-	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
-		!recurse && !recursing)
-		ereport(ERROR,
-(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be dropped from child tables too")));
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		PartitionDesc	partdesc = RelationGetPartitionDesc(rel);
+
+		Assert(partdesc != NULL);
+		if (partdesc->nparts > 0 && !recurse && !recursing)
+			ereport(ERROR,
+	(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+	 errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
+	 errhint("Do not specify the ONLY 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-16 Thread Noah Misch
On Thu, Apr 13, 2017 at 11:38:08AM -0400, Robert Haas wrote:
> On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost  wrote:
> > Sure, though I won't be able to today and I've got some doubts about the
> > other patches.  I'll have more time tomorrow though.
> 
> OK, cool.  I'll mark you down as the owner on the open items list.

[Action required within three days.]

The above-described topic is currently a PostgreSQL 10 open item.  Stephen, as
its owner, please observe the policy on open item ownership[1] and send a
status update within three calendar days of this message.  Include a date for
your subsequent status update.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-13 Thread Amit Langote
Hi Stephen,

On 2017/04/14 0:05, Stephen Frost wrote:
> Robert,
> 
> * Robert Haas (robertmh...@gmail.com) wrote:
>> So I think I was indeed confused before, and I think you're basically
>> right here, but on one point I think you are not right -- ALTER TABLE
>> ONLY .. CHECK () doesn't work on a table with inheritance children
>> regardless of whether the children already have the matching
>> constraint:
> 
> To be clear- I wasn't thinking about what's possible today with
> inheiritance or partitioning or in PG at all, but rather focusing on
> what a user is likely to expect and understand from the messaging.
> 
>> rhaas=# create table foo (a int, b text);
>> CREATE TABLE
>> rhaas=# create table bar () inherits (foo);
>> CREATE TABLE
>> rhaas=# alter table only foo add check (a = 1);
>> ERROR:  constraint must be added to child tables too
>> rhaas=# alter table only bar add check (a = 1);
>> ALTER TABLE
>> rhaas=# alter table only foo add check (a = 1);
>> ERROR:  constraint must be added to child tables too
> 
> If that's the case then we should really change that error message, in
> my view.  I'd be happier if we did support adding the constraint after
> child tables exist,

Do you mean to use ONLY even if the child tables exist and still succeed?
You can succeed in that case only by specifying NO INHERIT against the
constraint with traditional inheritance.  It does not work however with
partitioned tables, since we do not allow NO INHERIT constraints to be
defined on the partitioned tables; such a constraint would never be
enforced if we had allowed that and just sit there.  In the traditional
inheritance case, it is applied to the parent's own rows.

> but if we don't, we shouldn't imply to the user that
> they can add it after adding it to the children.

Hmm, the error message as it is *might* give the impression that we are
asking a user to go add the constraint to the child tables first.  Another
way the user might interpret the message is that it is asking to drop the
ONLY from the command (at least if they have read the documentation of
ONLY on the ALTER TABLE reference page).  But it perhaps isn't readily
apparent from the error message itself, so maybe a HINT message along
those lines might work.

>> So, regarding Amit's 0001:
>>
>> - I think we should update the relevant hunk of the documentation
>> rather than just removing it.
> 
> Alright.

You may have seen that the latest patch has taken care of this.  But maybe
you'll want to tweak my rewording further as you see fit.

>> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
>> .. to work on a partitioned table without partitions, or is that just
>> pointless tinkering?  That seems to be the only case where, after this
>> patch, an ONLY operation will fail on a childless partitioned table.
> 
> I'm less sure about TRUNCATE ONLY because that really isn't meaningful
> at all, is it?  A partitioned table doesn't have any data to truncate
> and truncating it doesn't have any impact on what happens later, does
> it?

That's right.  If you perform truncate (or truncate only) on an empty
partitioned table (i.e. with no partitions yet), it's essentially a no-op.
 Nor does it affect anything in the future.

> Having a sensible error be reported if someone tries to do that
> would be good though.

The latest patch implements the following:

-- create an empty partitioned table, aka without partitions
create table p (a int) partition by list (a);

-- no error, though nothing really happens
truncate only p;
TRUNCATE TABLE

-- add a partition
create table p1 partition of p for values in (1);

-- error, because a partition exists
truncate only p;
ERROR:  must truncate partitions too

-- ok, partition p1 will be truncated
truncate p;
TRUNCATE TABLE

I do now wonder if the error message "must truncate partitions *too*" is
somewhat inappropriate here.  The "too" seems to imply that table
specified in the command (which is partitioned) is somehow truncate-able,
which it is not.  Hmm.

>> Do you want to be responsible for committing these or should I do it?
> 
> Sure, though I won't be able to today and I've got some doubts about the
> other patches.  I'll have more time tomorrow though.

Thanks, I'll revise the patches per any review comments you might have.

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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-13 Thread Robert Haas
On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost  wrote:
> Sure, though I won't be able to today and I've got some doubts about the
> other patches.  I'll have more time tomorrow though.

OK, cool.  I'll mark you down as the owner on the open items list.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-13 Thread Stephen Frost
Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> So I think I was indeed confused before, and I think you're basically
> right here, but on one point I think you are not right -- ALTER TABLE
> ONLY .. CHECK () doesn't work on a table with inheritance children
> regardless of whether the children already have the matching
> constraint:

To be clear- I wasn't thinking about what's possible today with
inheiritance or partitioning or in PG at all, but rather focusing on
what a user is likely to expect and understand from the messaging.

> rhaas=# create table foo (a int, b text);
> CREATE TABLE
> rhaas=# create table bar () inherits (foo);
> CREATE TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too
> rhaas=# alter table only bar add check (a = 1);
> ALTER TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too

If that's the case then we should really change that error message, in
my view.  I'd be happier if we did support adding the constraint after
child tables exist, but if we don't, we shouldn't imply to the user that
they can add it after adding it to the children.

> So, regarding Amit's 0001:
> 
> - I think we should update the relevant hunk of the documentation
> rather than just removing it.

Alright.

> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
> .. to work on a partitioned table without partitions, or is that just
> pointless tinkering?  That seems to be the only case where, after this
> patch, an ONLY operation will fail on a childless partitioned table.

I'm less sure about TRUNCATE ONLY because that really isn't meaningful
at all, is it?  A partitioned table doesn't have any data to truncate
and truncating it doesn't have any impact on what happens later, does
it?  Having a sensible error be reported if someone tries to do that
would be good though.

> Do you want to be responsible for committing these or should I do it?

Sure, though I won't be able to today and I've got some doubts about the
other patches.  I'll have more time tomorrow though.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-12 Thread Amit Langote
On 2017/04/13 6:22, Robert Haas wrote:
> On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost  wrote:
>> I'm not following what you're getting at here.
>>
>> There's already a constraint on the table, and ALTER TABLE ONLY doesn't
>> say anything about what happens later on (certainly it doesn't make new
>> tables created with 'LIKE' have bits omitted, if that's what you were
>> thinking).  Lastly, the error being thrown certainly seems to imply that
>> one needs to go fix all the child tables to have the constraint first
>> and then the constraint can be added to the parent (presumably using the
>> same ALTER TABLE ONLY command).  If there aren't any child tables, then
>> it should work, if there *are* child tables and they've got the
>> necessary constraint, then this should be allowed, so that future child
>> tables create will have the constraint.
> 
> So I think I was indeed confused before, and I think you're basically
> right here, but on one point I think you are not right -- ALTER TABLE
> ONLY .. CHECK () doesn't work on a table with inheritance children
> regardless of whether the children already have the matching
> constraint:
> 
> rhaas=# create table foo (a int, b text);
> CREATE TABLE
> rhaas=# create table bar () inherits (foo);
> CREATE TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too
> rhaas=# alter table only bar add check (a = 1);
> ALTER TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too
> 
> It looks like ALTER TABLE ONLY works find on a table with no children,
> but once it's got children it no longer works, period.

By the way, there is a workaround with traditional inheritance:

alter table only foo add constraint chka check (a > 0) no inherit;
ALTER TABLE

But we don't allow NO INHERIT constraints on partitioned tables, so we
will get an error with them anyway.

alter table only parted_parent add constraint chka check (a > 0) no inherit;
ERROR:  cannot add NO INHERIT constraint to partitioned table "parted_parent"

> However,
> you're right that you can add the constraint to the as-yet-childless
> table and then future children will inherit the constraint properly.
> Continuing the previous example:
> 
> rhaas=# drop table bar;
> DROP TABLE
> rhaas=# alter table only foo add check (a = 1);
> ALTER TABLE
> rhaas=# create table bar () inherits (foo);
> CREATE TABLE
> 
> So, regarding Amit's 0001:
> 
> - I think we should update the relevant hunk of the documentation
> rather than just removing it.

OK, I agree.  I tweaked the existing bullet point about differences from
traditional inheritance when using ONLY with partitioned tables.

> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
> .. to work on a partitioned table without partitions, or is that just
> pointless tinkering?  That seems to be the only case where, after this
> patch, an ONLY operation will fail on a childless partitioned table.

I fixed TRUNCATE ONLY to not complain when no partitions exist.  Patch
already takes care of the ALTER TABLE ONLY cases.

Updated patches attached (0002 and 0003 unchanged).

Thanks,
Amit

>From 730a71a1120d37065e26338566fcbca65d1a03e8 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 12 Apr 2017 17:58:07 +0900
Subject: [PATCH 1/3] Fix ALTER TABLE ONLY to avoid unnecessarily failures

Currently, ALTER TABLE ONLY would fail in certain cases when applied
to partitioned tables, even if no partitions yet exist.  Although,
it seems perfectly OK to allow the same, especially because pg_dump
sometimes outputs constraints separately using ALTER TABLE ONLY.
---
 doc/src/sgml/ddl.sgml | 13 +++---
 src/backend/commands/tablecmds.c  | 69 +++
 src/test/regress/expected/alter_table.out | 32 +-
 src/test/regress/expected/truncate.out|  5 +++
 src/test/regress/sql/alter_table.sql  | 24 ---
 src/test/regress/sql/truncate.sql |  4 ++
 6 files changed, 98 insertions(+), 49 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 340c961b3f..ae2072b1de 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2944,17 +2944,18 @@ VALUES ('Albany', NULL, NULL, 'NY');
Both CHECK and NOT NULL
constraints of a partitioned table are always inherited by all its
partitions.  CHECK constraints that are marked
-   NO INHERIT are not allowed.
+   NO INHERIT are not allowed to be created on
+   partitioned tables.
   
  
 
  
   
-   The ONLY notation used to exclude child tables
-   will cause an error for partitioned tables in the case of
-   schema-modifying commands such as most ALTER TABLE
-   commands.  For example, dropping a column from only the parent does
-   not make sense for partitioned tables.
+   Using ONLY to affect only the 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-12 Thread Robert Haas
On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost  wrote:
> I'm not following what you're getting at here.
>
> There's already a constraint on the table, and ALTER TABLE ONLY doesn't
> say anything about what happens later on (certainly it doesn't make new
> tables created with 'LIKE' have bits omitted, if that's what you were
> thinking).  Lastly, the error being thrown certainly seems to imply that
> one needs to go fix all the child tables to have the constraint first
> and then the constraint can be added to the parent (presumably using the
> same ALTER TABLE ONLY command).  If there aren't any child tables, then
> it should work, if there *are* child tables and they've got the
> necessary constraint, then this should be allowed, so that future child
> tables create will have the constraint.

So I think I was indeed confused before, and I think you're basically
right here, but on one point I think you are not right -- ALTER TABLE
ONLY .. CHECK () doesn't work on a table with inheritance children
regardless of whether the children already have the matching
constraint:

rhaas=# create table foo (a int, b text);
CREATE TABLE
rhaas=# create table bar () inherits (foo);
CREATE TABLE
rhaas=# alter table only foo add check (a = 1);
ERROR:  constraint must be added to child tables too
rhaas=# alter table only bar add check (a = 1);
ALTER TABLE
rhaas=# alter table only foo add check (a = 1);
ERROR:  constraint must be added to child tables too

It looks like ALTER TABLE ONLY works find on a table with no children,
but once it's got children it no longer works, period.  However,
you're right that you can add the constraint to the as-yet-childless
table and then future children will inherit the constraint properly.
Continuing the previous example:

rhaas=# drop table bar;
DROP TABLE
rhaas=# alter table only foo add check (a = 1);
ALTER TABLE
rhaas=# create table bar () inherits (foo);
CREATE TABLE

So, regarding Amit's 0001:

- I think we should update the relevant hunk of the documentation
rather than just removing it.

- Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
.. to work on a partitioned table without partitions, or is that just
pointless tinkering?  That seems to be the only case where, after this
patch, an ONLY operation will fail on a childless partitioned table.

Do you want to be responsible for committing these or should I do it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-12 Thread Robert Haas
On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
>>  wrote:
>> > Actually, p1 is a partitioned table, so the error.  And I realize that
>> > that's a wrong behavior.  Currently the check is performed using only the
>> > relkind, which is bogus.  Specifying ONLY should cause an error only when
>> > the table has partitions.
>>
>> That sounds like a REALLY bad idea, because now you're going to end up
>> with a constraint that can never be enforced against any actual data
>> rows ... or else you're going to later pretend that ONLY wasn't
>> specified.  I think the rule that partitioned tables can't have
>> non-inherited constraints is absolutely right, and relaxing it is
>> quite wrong.
>
> I'm not following what you're getting at here.

Urk, I might be confusing ONLY with NO INHERIT.  Let me think about
this again...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-12 Thread Stephen Frost
Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
>  wrote:
> > Actually, p1 is a partitioned table, so the error.  And I realize that
> > that's a wrong behavior.  Currently the check is performed using only the
> > relkind, which is bogus.  Specifying ONLY should cause an error only when
> > the table has partitions.
> 
> That sounds like a REALLY bad idea, because now you're going to end up
> with a constraint that can never be enforced against any actual data
> rows ... or else you're going to later pretend that ONLY wasn't
> specified.  I think the rule that partitioned tables can't have
> non-inherited constraints is absolutely right, and relaxing it is
> quite wrong.

I'm not following what you're getting at here.

There's already a constraint on the table, and ALTER TABLE ONLY doesn't
say anything about what happens later on (certainly it doesn't make new
tables created with 'LIKE' have bits omitted, if that's what you were
thinking).  Lastly, the error being thrown certainly seems to imply that
one needs to go fix all the child tables to have the constraint first
and then the constraint can be added to the parent (presumably using the
same ALTER TABLE ONLY command).  If there aren't any child tables, then
it should work, if there *are* child tables and they've got the
necessary constraint, then this should be allowed, so that future child
tables create will have the constraint.

> I think you had the right idea upthread when you suggested dumping it this 
> way:
> 
> CREATE TABLE p1 PARTITION OF p (
> b NOT NULL
> )
> FOR VALUES IN (1)
> PARTITION BY RANGE (b);
> 
> That looks absolutely right to me, and very much principled.

It's not principled at all to claim that CREATE TABLE + NOT NULL is
somehow different from CREATE TABLE + ALTER TABLE SET NOT NULL and that
one should work and the other shouldn't.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-12 Thread Robert Haas
On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
 wrote:
> Actually, p1 is a partitioned table, so the error.  And I realize that
> that's a wrong behavior.  Currently the check is performed using only the
> relkind, which is bogus.  Specifying ONLY should cause an error only when
> the table has partitions.

That sounds like a REALLY bad idea, because now you're going to end up
with a constraint that can never be enforced against any actual data
rows ... or else you're going to later pretend that ONLY wasn't
specified.  I think the rule that partitioned tables can't have
non-inherited constraints is absolutely right, and relaxing it is
quite wrong.

I think you had the right idea upthread when you suggested dumping it this way:

CREATE TABLE p1 PARTITION OF p (
b NOT NULL
)
FOR VALUES IN (1)
PARTITION BY RANGE (b);

That looks absolutely right to me, and very much principled.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-12 Thread Amit Langote
Hi Stephen,

On 2017/04/11 22:12, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> On 2017/04/11 0:26, Robert Haas wrote:
>>> Children can have constraints (including NOT NULL constraints) which
>>> parents lack, and can have a different column order, but must have
>>> exactly the same column names and types.
>>
>> Also, what is different in the partitioned parent case is that NOT NULL
>> constraints must be inherited.  That is, one cannot add it only to the 
>> parent.
> 
> If I'm following, children can have additional constraints but any
> constraints on the parent must also exist on all the children.  Is that
> correct?

Yes.  As shown in the examples I posted, in traditional inheritance, only
CHECK constraints are treated that way, whereas NOT NULL constraints are
not.  With partitioning, even the NOT NULL constraints are inherited.  So
if a particular column is set NOT NULL in the parent, all partitions in
the tree must have that constraint and it cannot be dropped from a
partition without dropping it from the parent as well.  Traditional
inheritance applies that rule only to attributes and CHECK constraints.

>> --
>> -- partitioning inheritance
>> --
>> create table parted_parent (a int) partition by list (a);
>> create table part partition of parted_parent for values in (1);
>>
>> -- this is same as traditional inheritance
>> alter table only parted_parent add constraint chka check (a > 0);
>> -- ERROR:  constraint must be added to child tables too
> 
> Ok, this makes sense, but surely that constraint does, in fact, exist on
> the child already or we wouldn't be trying to dump out this constraint
> that exists on the parent?

Yes, that's right.

Now that I have thought about this a bit more, I think I can articulate
the problem statement more clearly.  The problem we have here is about
non-inherited constraints on partitions, specifically, the NOT NULL
constraints which need to be emitted per attribute. We have two options of
doing it: emit it inline with CREATE TABLE or separately with ALTER TABLE
ONLY.

I have been saying that it is preferable to use CREATE TABLE, because
ALTER TABLE ONLY will fail if the partition is itself partitioned.  Why
does it cause an error?  Because of the ONLY part.  It is a user error to
specify ONLY when adding a constraint to a partitioned tables, but...

>>> In Amit's example from the original post, the child has an implicit
>>> NOT NULL constraint that does not exist in the parent.  p1.b isn't
>>> declared NOT NULL, but the fact that it is range-partitioned on b
>>> requires it to be so, just as we would do if b were declared as the
>>> PRIMARY KEY.  Somehow that's not playing nice with pg_dump, but I'm
>>> still fuzzy on the details.
>>
>> Actually, I would like to change the problem definition from "ALTER TABLE
>> ONLY partitioned_table should be avoided" to "Emitting partition's
>> attributes separately should be avoided".
> 
> I don't follow why we think doing:
> 
> CREATE TABLE t1 (c1 int);
> ALTER TABLE ONLY t1 SET c1 NOT NULL;
> 
> is really different from:
> 
> CREATE TABLE t1 (c1 int NOT NULL);
> 
> or why we should teach pg_dump that it's "correct" to consider those two
> to be different.  There are specific cases where they have to be done
> independently, but that's for views because we don't have a way to set a
> default on a view column during CREATE VIEW, or to deal with dropped
> columns or traditionally inheirited columns.
> 
> What isn't clear to me is why the CREATE TABLE + ALTER TABLE isn't
> working, when apparently a CREATE TABLE with the NOT NULL included would
> work.  The issue here seems like it's the order in which the operations
> are happening in, and not that CREATE TABLE + ALTER TABLE is somehow
> different than just the CREATE TABLE.

I think you are right.  CREATE TABLE + ALTER TABLE ONLY should work just
fine in this particular case (i.e. the way pg_dump outputs it).

>> create table p (a int, b int) partition by list (a);
>> create table p1 partition of p for values in (1) partition by range (b);
>> create table p11 partition of p1 (
>> a not null default '1'
>> ) for values from (1) to (10);
> 
> Using the above example, doing a pg_dump and then a restore (into a
> clean initdb'd cluster), I get the following:
> 
> =# CREATE TABLE p (
> -# a integer,
> -# b integer
> -# )
> -# PARTITION BY LIST (a);
> CREATE TABLE
> 
> =*# CREATE TABLE p1 PARTITION OF p
> -*# FOR VALUES IN (1)
> -*# PARTITION BY RANGE (b);
> CREATE TABLE
> 
> =*# ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL;
> ERROR:  constraint must be added to child tables too
> 
> Now, perhaps I'm confused, but isn't p1 the child here?  Which is
> supposed to be able to have constraints that the parent doesn't?

Actually, p1 is a partitioned table, so the error.  And I realize that
that's a wrong behavior.  Currently the check is performed using only the
relkind, which is bogus.  Specifying ONLY should cause an error 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-11 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/04/11 0:26, Robert Haas wrote:
> > Children can have constraints (including NOT NULL constraints) which
> > parents lack, and can have a different column order, but must have
> > exactly the same column names and types.
> 
> Also, what is different in the partitioned parent case is that NOT NULL
> constraints must be inherited.  That is, one cannot add it only to the parent.

If I'm following, children can have additional constraints but any
constraints on the parent must also exist on all the children.  Is that
correct?

> --
> -- partitioning inheritance
> --
> create table parted_parent (a int) partition by list (a);
> create table part partition of parted_parent for values in (1);
> 
> -- this is same as traditional inheritance
> alter table only parted_parent add constraint chka check (a > 0);
> -- ERROR:  constraint must be added to child tables too

Ok, this makes sense, but surely that constraint does, in fact, exist on
the child already or we wouldn't be trying to dump out this constraint
that exists on the parent?

> > In Amit's example from the original post, the child has an implicit
> > NOT NULL constraint that does not exist in the parent.  p1.b isn't
> > declared NOT NULL, but the fact that it is range-partitioned on b
> > requires it to be so, just as we would do if b were declared as the
> > PRIMARY KEY.  Somehow that's not playing nice with pg_dump, but I'm
> > still fuzzy on the details.
> 
> Actually, I would like to change the problem definition from "ALTER TABLE
> ONLY partitioned_table should be avoided" to "Emitting partition's
> attributes separately should be avoided".

I don't follow why we think doing:

CREATE TABLE t1 (c1 int);
ALTER TABLE ONLY t1 SET c1 NOT NULL;

is really different from:

CREATE TABLE t1 (c1 int NOT NULL);

or why we should teach pg_dump that it's "correct" to consider those two
to be different.  There are specific cases where they have to be done
independently, but that's for views because we don't have a way to set a
default on a view column during CREATE VIEW, or to deal with dropped
columns or traditionally inheirited columns.

What isn't clear to me is why the CREATE TABLE + ALTER TABLE isn't
working, when apparently a CREATE TABLE with the NOT NULL included would
work.  The issue here seems like it's the order in which the operations
are happening in, and not that CREATE TABLE + ALTER TABLE is somehow
different than just the CREATE TABLE.

> create table p (a int, b int) partition by list (a);
> create table p1 partition of p for values in (1) partition by range (b);
> create table p11 partition of p1 (
> a not null default '1'
> ) for values from (1) to (10);

Using the above example, doing a pg_dump and then a restore (into a
clean initdb'd cluster), I get the following:

=# CREATE TABLE p (
-# a integer,
-# b integer
-# )
-# PARTITION BY LIST (a);
CREATE TABLE

=*# CREATE TABLE p1 PARTITION OF p
-*# FOR VALUES IN (1)
-*# PARTITION BY RANGE (b);
CREATE TABLE

=*# ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL;
ERROR:  constraint must be added to child tables too

Now, perhaps I'm confused, but isn't p1 the child here?  Which is
supposed to be able to have constraints that the parent doesn't?

We haven't even gotten to the point where p1 is a parent yet because p11
hasn't been created yet.  Further, according to psql's \d, 'p1.b'
already has a NOT NULL constraint on it, so the above really should just
be a no-op.

I get the feeling that we're looking in the wrong place for the issue
here.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-11 Thread Amit Langote
On 2017/04/11 0:26, Robert Haas wrote:
> On Sun, Apr 9, 2017 at 10:10 PM, Tom Lane  wrote:
>> While I admit that I've not been paying close attention to the whole
>> table partitioning business, I wonder whether we have any clearly written
>> down specification about (a) how much partition member tables are allowed
>> to deviate schema-wise from their parent, and (b) how DDL semantics on
>> partitioned tables differ from DDL semantics for traditional inheritance.
>> Obviously those are closely related questions.  But the fact that this
>> bug exists at all shows that there's been some lack of clarity on (b),
>> and so I wonder whether we have any clarity on (a) either.
> 
> Children can have constraints (including NOT NULL constraints) which
> parents lack, and can have a different column order, but must have
> exactly the same column names and types.

Also, what is different in the partitioned parent case is that NOT NULL
constraints must be inherited.  That is, one cannot add it only to the parent.

--
-- traditional inheritance
--
create table parent (a int);
create table child () inherits (parent);
alter table only parent add constraint chka check (a > 0);
-- ERROR:  constraint must be added to child tables too

-- the following is OK, because of the explicit NO INHERIT request
alter table only parent add constraint chka check (a > 0) no inherit;

-- the following is also OK, because NOT NULL constraints are not
-- inherited with regular inheritance
alter table only parent alter a set not null;

--
-- partitioning inheritance
--
create table parted_parent (a int) partition by list (a);
create table part partition of parted_parent for values in (1);

-- this is same as traditional inheritance
alter table only parted_parent add constraint chka check (a > 0);
-- ERROR:  constraint must be added to child tables too

-- requesting NO INHERIT doesn't make sense on what is an empty relation
-- by itself
alter table only parted_parent add constraint chka check (a > 0) no inherit;
-- ERROR:  cannot add NO INHERIT constraint to partitioned table
"parted_table"

-- NOT NULL constraint must be inherited too
alter table only parted_parent alter a set not null;
-- ERROR:  constraint must be added to child tables too

-- both ok (no inheritance here)
alter table part alter a set not null;
alter table part alter a drop not null;

-- request whole-tree constraint
alter table parted_parent alter a set not null;
-- can't drop it from a partition
alter table part alter a drop not null;
-- ERROR:  column "a" is marked NOT NULL in parent table

> In Amit's example from the original post, the child has an implicit
> NOT NULL constraint that does not exist in the parent.  p1.b isn't
> declared NOT NULL, but the fact that it is range-partitioned on b
> requires it to be so, just as we would do if b were declared as the
> PRIMARY KEY.  Somehow that's not playing nice with pg_dump, but I'm
> still fuzzy on the details.

Actually, I would like to change the problem definition from "ALTER TABLE
ONLY partitioned_table should be avoided" to "Emitting partition's
attributes separately should be avoided".

There is a shouldPrintColumn() which returns true if a table's attribute
should be emitted in the CREATE TABLE command at all.  For example, it
won't be necessary if the attribute is purely inherited
(attislocal=false).  But once an attribute is determined to not be
emitted, any attribute-level constraints and defaults need to be marked to
be emitted separately (using ALTER TABLE ONLY), which can be undesirable
because of the rules about using ONLY with declarative partitioning
inheritance.

I think we can change shouldPrintColumn() so that it always returns true
if the table is a partition, that is, a partition's attributes should
always be emitted with CREATE TABLE, if necessary (it isn't required if
there isn't a NOT NULL constraint or DEFAULT defined on the attributes).
When dumping a partition using the PARTITION OF syntax, an attribute will
emitted if it has a NOT NULL constraint or a default.

So if we have:

create table p (a int, b int) partition by list (a);
create table p1 partition of p for values in (1) partition by range (b);
create table p11 partition of p1 (
a not null default '1'
) for values from (1) to (10);

Proposed would make pg_dump -s output the following (some lines removed
for clarity):

CREATE TABLE p (
a integer,
b integer
)
PARTITION BY LIST (a);

CREATE TABLE p1 PARTITION OF p (
b NOT NULL
)
FOR VALUES IN (1)
PARTITION BY RANGE (b);

CREATE TABLE p11 PARTITION OF p1 (
a DEFAULT 1 NOT NULL,
b NOT NULL
)
FOR VALUES FROM (1) TO (10);

Which also happens to a legal input to the backend, as far as partitioning
is concerned.  With the existing approach, NOT NULL on p1's b attribute is
emitted as ALTER TABLE ONLY p1 ALTER b SET NOT NULL, which would cause
error, because p1 is partitioned.

Attached patch implements that and also updates some comments.  I think
some 

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-10 Thread Robert Haas
On Sun, Apr 9, 2017 at 10:10 PM, Tom Lane  wrote:
> While I admit that I've not been paying close attention to the whole
> table partitioning business, I wonder whether we have any clearly written
> down specification about (a) how much partition member tables are allowed
> to deviate schema-wise from their parent, and (b) how DDL semantics on
> partitioned tables differ from DDL semantics for traditional inheritance.
> Obviously those are closely related questions.  But the fact that this
> bug exists at all shows that there's been some lack of clarity on (b),
> and so I wonder whether we have any clarity on (a) either.

Children can have constraints (including NOT NULL constraints) which
parents lack, and can have a different column order, but must have
exactly the same column names and types.

The point here is, of course, not that there's any real value in the
parent columns being (a, b) and the child columns being (b, a),
although we do allow that, but rather than somebody might have a
parent with (a, b) and a child that has those plus a dropped column.
Explaining to a user - to whom the dropped column is invisible - why
that child couldn't be attached as a partition of that parent would be
difficult, so it seemed best (to me, anyway, and I think to other
people who were paying attention) to rule that the partitioning code
has to cope with the possibility of attribute numbers varying across
partitions.  (Also consider the reverse case, where the parent has a
dropped column and the prospective child doesn't have one with the
same width in the same location.)

In Amit's example from the original post, the child has an implicit
NOT NULL constraint that does not exist in the parent.  p1.b isn't
declared NOT NULL, but the fact that it is range-partitioned on b
requires it to be so, just as we would do if b were declared as the
PRIMARY KEY.  Somehow that's not playing nice with pg_dump, but I'm
still fuzzy on the details.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-10 Thread Tom Lane
Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> While I admit that I've not been paying close attention to the whole
>> table partitioning business, I wonder whether we have any clearly written
>> down specification about (a) how much partition member tables are allowed
>> to deviate schema-wise from their parent, and (b) how DDL semantics on
>> partitioned tables differ from DDL semantics for traditional inheritance.

> Right.  I thought I had understood that partition child tables really
> can't deviate from the parent table in terms of columns or constraints
> but might be allowed to differ with regard to indexes (?).

Well, there's an awful lot of logic that's only of interest if partition
children can have different column orders from their parents.  I really
fail to understand what's the point of allowing that.

regards, tom lane


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-10 Thread Stephen Frost
Tom, Robert,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > I would appreciate help from other contributors and committers on this
> > open item; pg_dump is not my strong point.  In the absence of such
> > help, I will do my best with it.  I will set aside time this week to
> > study this and send another update no later than Thursday.
> 
> The proposed patch seems rather ad-hoc, and I think that it is working
> around a backend behavior that might be broken.

Yeah, I'm not a big fan of the proposed patch either.

> While I admit that I've not been paying close attention to the whole
> table partitioning business, I wonder whether we have any clearly written
> down specification about (a) how much partition member tables are allowed
> to deviate schema-wise from their parent, and (b) how DDL semantics on
> partitioned tables differ from DDL semantics for traditional inheritance.
> Obviously those are closely related questions.  But the fact that this
> bug exists at all shows that there's been some lack of clarity on (b),
> and so I wonder whether we have any clarity on (a) either.

Right.  I thought I had understood that partition child tables really
can't deviate from the parent table in terms of columns or constraints
but might be allowed to differ with regard to indexes (?).

I'll try looking into this also, I should be able to spend some time on
it this week.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-09 Thread Tom Lane
Robert Haas  writes:
> I would appreciate help from other contributors and committers on this
> open item; pg_dump is not my strong point.  In the absence of such
> help, I will do my best with it.  I will set aside time this week to
> study this and send another update no later than Thursday.

The proposed patch seems rather ad-hoc, and I think that it is working
around a backend behavior that might be broken.

While I admit that I've not been paying close attention to the whole
table partitioning business, I wonder whether we have any clearly written
down specification about (a) how much partition member tables are allowed
to deviate schema-wise from their parent, and (b) how DDL semantics on
partitioned tables differ from DDL semantics for traditional inheritance.
Obviously those are closely related questions.  But the fact that this
bug exists at all shows that there's been some lack of clarity on (b),
and so I wonder whether we have any clarity on (a) either.

regards, tom lane


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-09 Thread Robert Haas
On Sun, Apr 9, 2017 at 7:50 PM, Noah Misch  wrote:
> The above-described topic is currently a PostgreSQL 10 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

I would appreciate help from other contributors and committers on this
open item; pg_dump is not my strong point.  In the absence of such
help, I will do my best with it.  I will set aside time this week to
study this and send another update no later than Thursday.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-04-09 Thread Noah Misch
On Wed, Mar 29, 2017 at 05:38:41PM +0900, Amit Langote wrote:
> On 2017/03/29 0:39, Robert Haas wrote:
> > On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote
> >  wrote:
> >>> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL
> >>> columns at all?  You didn't say anything like that when setting up the
> >>> database, so why should it be there when dumping?
> >>
> >> So we should find a way for the NOT NULL constraints added for the range
> >> partition key columns to not be emitted *separately*?  Like when a table
> >> has primary key:
> >>
> >> --
> >> -- Name: foo; Type: TABLE; Schema: public; Owner: amit
> >> --
> >>
> >> CREATE TABLE foo (
> >> a integer NOT NULL
> >> );
> >>
> >>
> >> ALTER TABLE foo OWNER TO amit;
> >>
> >> --
> >> -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit
> >> --
> >>
> >> ALTER TABLE ONLY foo
> >> ADD CONSTRAINT foo_pkey PRIMARY KEY (a);
> >>
> >> The NOT NULL constraint is emitted with CREATE TABLE, not separately.
> > 
> > Hmm, that's not exactly what I was thinking, but I think what I was
> > thinking was wrong, so, yes, can we do what you said?
> 
> I thought about this for a while.  Although it seems we can do what I said
> for (partitioned) tables themselves, it's not real clear to me how
> straightforward it is to do for partitions (child tables). Inheritance or
> localness of attributes/constraints including NOT NULL dictates whether an
> attribute or a constraint is emitted separately.  I think that any
> additional consideration will make the logic to *not* dump separately (or
> perhaps to not emit at all) will become more involved.  For example, if a
> NOT NULL constraint on a column has been inherited and originated in the
> parent from the range partition key, then does it mean we should not emit
> it or emit it but not separately?

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-03-29 Thread Amit Langote
On 2017/03/29 0:39, Robert Haas wrote:
> On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote
>  wrote:
>>> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL
>>> columns at all?  You didn't say anything like that when setting up the
>>> database, so why should it be there when dumping?
>>
>> So we should find a way for the NOT NULL constraints added for the range
>> partition key columns to not be emitted *separately*?  Like when a table
>> has primary key:
>>
>> --
>> -- Name: foo; Type: TABLE; Schema: public; Owner: amit
>> --
>>
>> CREATE TABLE foo (
>> a integer NOT NULL
>> );
>>
>>
>> ALTER TABLE foo OWNER TO amit;
>>
>> --
>> -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit
>> --
>>
>> ALTER TABLE ONLY foo
>> ADD CONSTRAINT foo_pkey PRIMARY KEY (a);
>>
>> The NOT NULL constraint is emitted with CREATE TABLE, not separately.
> 
> Hmm, that's not exactly what I was thinking, but I think what I was
> thinking was wrong, so, yes, can we do what you said?

I thought about this for a while.  Although it seems we can do what I said
for (partitioned) tables themselves, it's not real clear to me how
straightforward it is to do for partitions (child tables). Inheritance or
localness of attributes/constraints including NOT NULL dictates whether an
attribute or a constraint is emitted separately.  I think that any
additional consideration will make the logic to *not* dump separately (or
perhaps to not emit at all) will become more involved.  For example, if a
NOT NULL constraint on a column has been inherited and originated in the
parent from the range partition key, then does it mean we should not emit
it or emit it but not separately?

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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-03-28 Thread Robert Haas
On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote
 wrote:
>> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL
>> columns at all?  You didn't say anything like that when setting up the
>> database, so why should it be there when dumping?
>
> So we should find a way for the NOT NULL constraints added for the range
> partition key columns to not be emitted *separately*?  Like when a table
> has primary key:
>
> --
> -- Name: foo; Type: TABLE; Schema: public; Owner: amit
> --
>
> CREATE TABLE foo (
> a integer NOT NULL
> );
>
>
> ALTER TABLE foo OWNER TO amit;
>
> --
> -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit
> --
>
> ALTER TABLE ONLY foo
> ADD CONSTRAINT foo_pkey PRIMARY KEY (a);
>
> The NOT NULL constraint is emitted with CREATE TABLE, not separately.

Hmm, that's not exactly what I was thinking, but I think what I was
thinking was wrong, so, yes, can we do what you said?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-03-28 Thread Amit Langote
On 2017/03/27 23:30, Robert Haas wrote:
> On Fri, Feb 17, 2017 at 3:23 AM, Amit Langote
>  wrote:
>> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
>> command for those schema elements of a table that could not be included
>> directly in the CREATE TABLE command for the table.
>>
>> For example:
>>
>> create table p (a int, b int) partition by range (a);
>> create table p1 partition of p for values from (1) to (10) partition by
>> range (b);
>> create table p11 partition of p1 for values from (1) to (10);
>>
>> pg_dump -s gives:
>>
>> CREATE TABLE p (
>> a integer NOT NULL,
>> b integer
>> )
>> PARTITION BY RANGE (a);
>>
>> CREATE TABLE p1 PARTITION OF p
>> FOR VALUES FROM (1) TO (10)
>> PARTITION BY RANGE (b);
>> ALTER TABLE ONLY p1 ALTER COLUMN a SET NOT NULL;
>> ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL;
>>
>> 
>>
>> Note the ONLY in the above emitted command.  Now if I run the above
>> commands in another database, the following error occurs:
>>
>> ERROR:  constraint must be added to child tables too
>>
>> That's because specifying ONLY for the AT commands on partitioned tables
>> that must recurse causes an error.
>>
>> Attached patch fixes that - it prevents emitting ONLY for those ALTER
>> TABLE commands, which if run, would cause an error like the one above.
> 
> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL
> columns at all?  You didn't say anything like that when setting up the
> database, so why should it be there when dumping?

So we should find a way for the NOT NULL constraints added for the range
partition key columns to not be emitted *separately*?  Like when a table
has primary key:

--
-- Name: foo; Type: TABLE; Schema: public; Owner: amit
--

CREATE TABLE foo (
a integer NOT NULL
);


ALTER TABLE foo OWNER TO amit;

--
-- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit
--

ALTER TABLE ONLY foo
ADD CONSTRAINT foo_pkey PRIMARY KEY (a);

The NOT NULL constraint is emitted with CREATE TABLE, not separately.

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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-03-28 Thread Amit Langote
Hi Stephen,

On 2017/03/21 1:40, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> On 2017/02/17 22:32, Stephen Frost wrote:
>>> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
 In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
 command for those schema elements of a table that could not be included
 directly in the CREATE TABLE command for the table.
>>>
>>> Any chance we could start adding regression tests for how pg_dump
>>> handles partitions?  I'm just about to the point where I have pretty
>>> much everything else covered (at least in pg_dump.c, where it's not a
>>> hard-to-reproduce error/exit case, or something version-dependent).
>>>
>>> If you have any questions about how the TAP tests for pg_dump work, or
>>> about how to generate code-coverage checks to make sure you're at least
>>> hitting every line (tho, of course, not every possible path), let me
>>> know.  I'd be happy to explain them.
>>
>> Yeah, I guess it would be a good idea to have some pg_dump TAP test
>> coverage for the new partitioning stuff.  I will look into that and get
>> back to you if I don't grok something there.
> 
> As you may have seen, I've added some tests to the pg_dump TAP tests for
> partitioning to cover lines of code not already covered.  There are
> still some bits not covered though, which you can see here:
> 
> https://coverage.postgresql.org/src/bin/pg_dump/pg_dump.c.gcov.html
> 
> If you have any questions about the way the pg_dump tests work, feel
> free to ask.

Sorry that it took me week to thank you for doing this.

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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-03-27 Thread Robert Haas
On Fri, Feb 17, 2017 at 3:23 AM, Amit Langote
 wrote:
> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
> command for those schema elements of a table that could not be included
> directly in the CREATE TABLE command for the table.
>
> For example:
>
> create table p (a int, b int) partition by range (a);
> create table p1 partition of p for values from (1) to (10) partition by
> range (b);
> create table p11 partition of p1 for values from (1) to (10);
>
> pg_dump -s gives:
>
> CREATE TABLE p (
> a integer NOT NULL,
> b integer
> )
> PARTITION BY RANGE (a);
>
> CREATE TABLE p1 PARTITION OF p
> FOR VALUES FROM (1) TO (10)
> PARTITION BY RANGE (b);
> ALTER TABLE ONLY p1 ALTER COLUMN a SET NOT NULL;
> ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL;
>
> 
>
> Note the ONLY in the above emitted command.  Now if I run the above
> commands in another database, the following error occurs:
>
> ERROR:  constraint must be added to child tables too
>
> That's because specifying ONLY for the AT commands on partitioned tables
> that must recurse causes an error.
>
> Attached patch fixes that - it prevents emitting ONLY for those ALTER
> TABLE commands, which if run, would cause an error like the one above.

Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL
columns at all?  You didn't say anything like that when setting up the
database, so why should it be there when dumping?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-03-20 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/02/17 22:32, Stephen Frost wrote:
> > * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> >> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
> >> command for those schema elements of a table that could not be included
> >> directly in the CREATE TABLE command for the table.
> > 
> > Any chance we could start adding regression tests for how pg_dump
> > handles partitions?  I'm just about to the point where I have pretty
> > much everything else covered (at least in pg_dump.c, where it's not a
> > hard-to-reproduce error/exit case, or something version-dependent).
> > 
> > If you have any questions about how the TAP tests for pg_dump work, or
> > about how to generate code-coverage checks to make sure you're at least
> > hitting every line (tho, of course, not every possible path), let me
> > know.  I'd be happy to explain them.
> 
> Yeah, I guess it would be a good idea to have some pg_dump TAP test
> coverage for the new partitioning stuff.  I will look into that and get
> back to you if I don't grok something there.

As you may have seen, I've added some tests to the pg_dump TAP tests for
partitioning to cover lines of code not already covered.  There are
still some bits not covered though, which you can see here:

https://coverage.postgresql.org/src/bin/pg_dump/pg_dump.c.gcov.html

If you have any questions about the way the pg_dump tests work, feel
free to ask.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-02-19 Thread Amit Langote
Hi Stephen,

On 2017/02/17 22:32, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
>> command for those schema elements of a table that could not be included
>> directly in the CREATE TABLE command for the table.
> 
> Any chance we could start adding regression tests for how pg_dump
> handles partitions?  I'm just about to the point where I have pretty
> much everything else covered (at least in pg_dump.c, where it's not a
> hard-to-reproduce error/exit case, or something version-dependent).
> 
> If you have any questions about how the TAP tests for pg_dump work, or
> about how to generate code-coverage checks to make sure you're at least
> hitting every line (tho, of course, not every possible path), let me
> know.  I'd be happy to explain them.

Yeah, I guess it would be a good idea to have some pg_dump TAP test
coverage for the new partitioning stuff.  I will look into that and get
back to you if I don't grok something there.

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] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-02-17 Thread Stephen Frost
Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
> command for those schema elements of a table that could not be included
> directly in the CREATE TABLE command for the table.

Any chance we could start adding regression tests for how pg_dump
handles partitions?  I'm just about to the point where I have pretty
much everything else covered (at least in pg_dump.c, where it's not a
hard-to-reproduce error/exit case, or something version-dependent).

If you have any questions about how the TAP tests for pg_dump work, or
about how to generate code-coverage checks to make sure you're at least
hitting every line (tho, of course, not every possible path), let me
know.  I'd be happy to explain them.

Thanks!

Stephen


signature.asc
Description: Digital signature


[HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

2017-02-17 Thread Amit Langote
In certain cases, pg_dump's dumpTableSchema() emits a separate ALTER TABLE
command for those schema elements of a table that could not be included
directly in the CREATE TABLE command for the table.

For example:

create table p (a int, b int) partition by range (a);
create table p1 partition of p for values from (1) to (10) partition by
range (b);
create table p11 partition of p1 for values from (1) to (10);

pg_dump -s gives:

CREATE TABLE p (
a integer NOT NULL,
b integer
)
PARTITION BY RANGE (a);

CREATE TABLE p1 PARTITION OF p
FOR VALUES FROM (1) TO (10)
PARTITION BY RANGE (b);
ALTER TABLE ONLY p1 ALTER COLUMN a SET NOT NULL;
ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL;



Note the ONLY in the above emitted command.  Now if I run the above
commands in another database, the following error occurs:

ERROR:  constraint must be added to child tables too

That's because specifying ONLY for the AT commands on partitioned tables
that must recurse causes an error.

Attached patch fixes that - it prevents emitting ONLY for those ALTER
TABLE commands, which if run, would cause an error like the one above.

Thanks,
Amit
>From a1b73a072ee366d8473289ed208b3bf9e7190312 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 14 Feb 2017 10:48:26 +0900
Subject: [PATCH] pg_dump: do not emit ALTER TABLE ONLY for partitioned tables

At least for cases where the inheritance recursion must take place,
such as dropping a column, adding a constraint, setting a column
NOT NULL.
---
 src/bin/pg_dump/pg_dump.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7364a12c25..a077606b85 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15114,10 +15114,13 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 	appendStringLiteralAH(q, fmtId(tbinfo->dobj.name), fout);
 	appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
 
-	if (tbinfo->relkind == RELKIND_RELATION ||
-		tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
+	if (tbinfo->relkind == RELKIND_RELATION)
 		appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
 		  fmtId(tbinfo->dobj.name));
+	/* attribute must be dropped in partitions too */
+	else if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
+		appendPQExpBuffer(q, "ALTER TABLE %s ",
+		  fmtId(tbinfo->dobj.name));
 	else
 		appendPQExpBuffer(q, "ALTER FOREIGN TABLE ONLY %s ",
 		  fmtId(tbinfo->dobj.name));
@@ -15145,8 +15148,12 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 	continue;
 
 appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inherited constraint.\n");
-appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
-  fmtId(tbinfo->dobj.name));
+if (tbinfo->relkind == RELKIND_RELATION)
+	appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+	  fmtId(tbinfo->dobj.name));
+else if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
+	appendPQExpBuffer(q, "ALTER TABLE %s ",
+	  fmtId(tbinfo->dobj.name));
 appendPQExpBuffer(q, " ADD CONSTRAINT %s ",
   fmtId(constr->dobj.name));
 appendPQExpBuffer(q, "%s;\n", constr->condef);
@@ -15187,7 +15194,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 			if (tbinfo->partitionOf)
 			{
 appendPQExpBufferStr(q, "\n-- For binary upgrade, set up partitions this way.\n");
-appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+appendPQExpBuffer(q, "ALTER TABLE %s ",
   fmtId(tbinfo->partitionOf->dobj.name));
 appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
   fmtId(tbinfo->dobj.name),
@@ -15249,8 +15256,12 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 			if (!shouldPrintColumn(dopt, tbinfo, j) &&
 tbinfo->notnull[j] && !tbinfo->inhNotNull[j])
 			{
-appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
-  fmtId(tbinfo->dobj.name));
+if (tbinfo->relkind == RELKIND_RELATION)
+	appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+	  fmtId(tbinfo->dobj.name));
+else if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
+	appendPQExpBuffer(q, "ALTER TABLE %s ",
+	  fmtId(tbinfo->dobj.name));
 appendPQExpBuffer(q, "ALTER COLUMN %s SET NOT NULL;\n",
   fmtId(tbinfo->attnames[j]));
 			}
-- 
2.11.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers