Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d3kGf-0001E5-K6 for pgsql-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 14:19:37 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d3kGf-0002HG-6a for pgsql-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 14:19:37 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1d3kF9-0007eP-Lg for pgsql-hackers@postgresql.org; Thu, 27 Apr 2017 14:18:03 +0000 Received: from tamriel.snowman.net ([72.66.115.51]) by magus.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d3kF3-0008BG-CW for pgsql-hackers@postgresql.org; Thu, 27 Apr 2017 14:18:02 +0000 Received: by tamriel.snowman.net (Postfix, from userid 1000) id C00495F794; Thu, 27 Apr 2017 10:17:26 -0400 (EDT) Date: Thu, 27 Apr 2017 10:17:26 -0400 From: Stephen Frost To: Amit Langote Cc: Robert Haas , Tom Lane , Noah Misch , Pg Hackers Subject: Re: pg_dump emits ALTER TABLE ONLY partitioned_table Message-ID: <20170427141726.GF21223@tamriel.snowman.net> References: <0fae1d55-337b-d7e1-d420-b3a617fc7383@lab.ntt.co.jp> <20170417164325.GY9812@tamriel.snowman.net> <399b8962-b761-528e-1008-a9643371c984@lab.ntt.co.jp> <20170420234334.GW9812@tamriel.snowman.net> <9b7baae3-21b5-a730-1ec4-aee290d54518@lab.ntt.co.jp> <20170425154247.GJ9812@tamriel.snowman.net> <5cdfd834-695a-9c05-8fb4-cde08de21a9b@lab.ntt.co.jp> <20170426143157.GA21223@tamriel.snowman.net> <47e1d272-8ce0-dea8-2bb5-642efa95c337@lab.ntt.co.jp> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Cgrdyab2wu3Akvjd" Content-Disposition: inline In-Reply-To: <47e1d272-8ce0-dea8-2bb5-642efa95c337@lab.ntt.co.jp> X-Editor: Vim http://www.vim.org/ X-Info: http://www.snowman.net X-Operating-System: Linux/3.13.0-91-generic (x86_64) X-Uptime: 09:06:24 up 292 days, 14:30, 37 users, load average: 0.12, 0.12, 0.13 User-Agent: Mutt/1.5.21 (2010-09-15) X-Pg-Spam-Score: -1.9 (-) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org --Cgrdyab2wu3Akvjd Content-Type: multipart/mixed; boundary="CXFpZVxO6m2Ol4tQ" Content-Disposition: inline --CXFpZVxO6m2Ol4tQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Amit, * Amit Langote (Langote_Amit_f8@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 colu= mns > >=20 > > 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? >=20 > OK, I think optionally allowing WITH OPTIONS keywords would be nice. >=20 > 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: > >=20 > > appendPQExpBuffer(q, "ALTER TABLE ONLY %s ", > > fmtId(tbinfo->partitionOf->dobj.name)= ); > > appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n", > > fmtId(tbinfo->dobj.name), > > tbinfo->partitiondef); > >=20 > > unlike the existing inheritance code a few lines above, which does: > >=20 > > appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ", > > fmtId(tbinfo->dobj.name)); > > if (parentRel->dobj.namespace !=3D tbinfo->dobj.nam= espace) > > appendPQExpBuffer(q, "%s.", > > fmtId(parentRel->dobj.namespace->dobj.n= ame)); > > appendPQExpBuffer(q, "%s;\n", > > fmtId(parentRel->dobj.name)); >=20 > 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.) > >=20 > > 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. >=20 > 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 --CXFpZVxO6m2Ol4tQ Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="make-with-options-optional_v1-master.patch" Content-Transfer-Encoding: quoted-printable =46rom 1d997077fb4e5395fed0e8e0dd7dc3b629a11553 Mon Sep 17 00:00:00 2001 =46rom: 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. This change simplifies the syntax and makes WITH OPTIONS optional when adding defaults, constraints or storage parameters to columns when creating either typed tables or partitions. Also update pg_dump to no longer include WITH OPTIONS, since it's not necessary, and update the documentation to reflect that WITH OPTIONS is now optional. --- doc/src/sgml/ref/create_foreign_table.sgml | 2 +- doc/src/sgml/ref/create_table.sgml | 4 +- src/backend/parser/gram.y | 68 ++++++++++----------------= ---- src/bin/pg_dump/pg_dump.c | 17 +++++--- 4 files changed, 36 insertions(+), 55 deletions(-) diff --git a/doc/src/sgml/ref/create_foreign_table.sgml b/doc/src/sgml/ref/= create_foreign_table.sgml index 5d0dcf5..065c982 100644 --- a/doc/src/sgml/ref/create_foreign_table.sgml +++ b/doc/src/sgml/ref/create_foreign_table.sgml @@ -29,7 +29,7 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] table_name =20 CREATE FOREIGN TABLE [ IF NOT EXISTS ] ta= ble_name PARTITION OF parent_table= [ ( - { column_name WITH OPTION= S [ column_constraint [ ... = ] ] + { column_name [ WITH OPTI= ONS ] [ column_constraint [ = =2E.. ] ] | table_constraint } [, ... ] ) ] partition_bound_spec diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_t= able.sgml index a3dc744..484f818 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -35,7 +35,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGG= ED ] TABLE [ IF NOT EXI =20 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF N= OT EXISTS ] table_name OF type_name [ ( - { column_name WITH OPTION= S [ column_constraint [ ... = ] ] + { column_name [ WITH OPTI= ONS ] [ column_constraint [ = =2E.. ] ] | table_constraint } [, ... ] ) ] @@ -46,7 +46,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGG= ED ] TABLE [ IF NOT EXI =20 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF N= OT EXISTS ] table_name PARTITION OF parent_table [ ( - { column_name [ column_constraint [ ... ] ] + { column_name [ WITH OPTI= ONS ] [ column_constraint [ = =2E.. ] ] | table_constraint } [, ... ] ) ] FOR VALUES partition_bound_spec diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 89d2836..21cdc7c 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -576,8 +576,6 @@ static Node *makeRecursiveViewSelect(char *relname, Lis= t *aliases, Node *query); %type part_strategy %type part_elem %type part_params -%type OptPartitionElementList PartitionElementList -%type PartitionElement %type ForValues %type partbound_datum %type partbound_datum_list @@ -3131,7 +3129,7 @@ CreateStmt: CREATE OptTemp TABLE qualified_name '(' O= ptTableElementList ')' $$ =3D (Node *)n; } | CREATE OptTemp TABLE qualified_name PARTITION OF qualified_name - OptPartitionElementList ForValues OptPartitionSpec OptWith + OptTypedTableElementList ForValues OptPartitionSpec OptWith OnCommitOption OptTableSpace { CreateStmt *n =3D makeNode(CreateStmt); @@ -3150,7 +3148,7 @@ CreateStmt: CREATE OptTemp TABLE qualified_name '(' O= ptTableElementList ')' $$ =3D (Node *)n; } | CREATE OptTemp TABLE IF_P NOT EXISTS qualified_name PARTITION OF - qualified_name OptPartitionElementList ForValues OptPartitionSpec + qualified_name OptTypedTableElementList ForValues OptPartitionSpec OptWith OnCommitOption OptTableSpace { CreateStmt *n =3D makeNode(CreateStmt); @@ -3213,11 +3211,6 @@ OptTypedTableElementList: | /*EMPTY*/ { $$ =3D NIL; } ; =20 -OptPartitionElementList: - '(' PartitionElementList ')' { $$ =3D $2; } - | /*EMPTY*/ { $$ =3D NIL; } - ; - TableElementList: TableElement { @@ -3240,17 +3233,6 @@ TypedTableElementList: } ; =20 -PartitionElementList: - PartitionElement - { - $$ =3D list_make1($1); - } - | PartitionElementList ',' PartitionElement - { - $$ =3D lappend($1, $3); - } - ; - TableElement: columnDef { $$ =3D $1; } | TableLikeClause { $$ =3D $1; } @@ -3262,28 +3244,6 @@ TypedTableElement: | TableConstraint { $$ =3D $1; } ; =20 -PartitionElement: - TableConstraint { $$ =3D $1; } - | ColId ColQualList - { - ColumnDef *n =3D makeNode(ColumnDef); - n->colname =3D $1; - n->typeName =3D NULL; - n->inhcount =3D 0; - n->is_local =3D true; - n->is_not_null =3D false; - n->is_from_type =3D false; - n->storage =3D 0; - n->raw_default =3D NULL; - n->cooked_default =3D NULL; - n->collOid =3D InvalidOid; - SplitColQualList($2, &n->constraints, &n->collClause, - yyscanner); - n->location =3D @1; - $$ =3D (Node *) n; - } - ; - columnDef: ColId Typename create_generic_options ColQualList { ColumnDef *n =3D makeNode(ColumnDef); @@ -3305,7 +3265,25 @@ columnDef: ColId Typename create_generic_options Col= QualList } ; =20 -columnOptions: ColId WITH OPTIONS ColQualList +columnOptions: ColId ColQualList + { + ColumnDef *n =3D makeNode(ColumnDef); + n->colname =3D $1; + n->typeName =3D NULL; + n->inhcount =3D 0; + n->is_local =3D true; + n->is_not_null =3D false; + n->is_from_type =3D false; + n->storage =3D 0; + n->raw_default =3D NULL; + n->cooked_default =3D NULL; + n->collOid =3D InvalidOid; + SplitColQualList($2, &n->constraints, &n->collClause, + yyscanner); + n->location =3D @1; + $$ =3D (Node *)n; + } + | ColId WITH OPTIONS ColQualList { ColumnDef *n =3D makeNode(ColumnDef); n->colname =3D $1; @@ -4872,7 +4850,7 @@ CreateForeignTableStmt: $$ =3D (Node *) n; } | CREATE FOREIGN TABLE qualified_name - PARTITION OF qualified_name OptPartitionElementList ForValues + PARTITION OF qualified_name OptTypedTableElementList ForValues SERVER name create_generic_options { CreateForeignTableStmt *n =3D makeNode(CreateForeignTableStmt); @@ -4893,7 +4871,7 @@ CreateForeignTableStmt: $$ =3D (Node *) n; } | CREATE FOREIGN TABLE IF_P NOT EXISTS qualified_name - PARTITION OF qualified_name OptPartitionElementList ForValues + PARTITION OF qualified_name OptTypedTableElementList ForValues SERVER name create_generic_options { CreateForeignTableStmt *n =3D makeNode(CreateForeignTableStmt); diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index e9b5c8a..2fda350 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -15267,13 +15267,16 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) continue; } =20 - /* Attribute type */ - if ((tbinfo->reloftype || tbinfo->partitionOf) && - !dopt->binary_upgrade) - { - appendPQExpBufferStr(q, " WITH OPTIONS"); - } - else + /* + * Attribute type + * + * In binary-upgrade mode, we always include the type. + * If we aren't in binary-upgrade mode, then we skip the + * type when creating a typed table ('OF type_name') or a + * partition ('PARTITION OF'), since the type comes from + * the parent/partitioned table. + */ + if (dopt->binary_upgrade || (!tbinfo->reloftype && !tbinfo->partition= Of)) { appendPQExpBuffer(q, " %s", tbinfo->atttypnames[j]); --=20 2.7.4 --CXFpZVxO6m2Ol4tQ-- --Cgrdyab2wu3Akvjd Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJZAf12AAoJEO1sijiDR2RVDXYP/01R7xRw7RD8B9SQ9W5eenKB 8Bj7oHgKrrXO9lV8k4T7nUsU0NhZowNj8azLM9tpdvrk63K4gnyRjMzene0+d+hD WO0Heq6t3ZONc4nzpU2IpX48oORD7IGnVdDOunOZBGpDPfdgsDLFZz0UZ66iazJC lETuKVgyzoHTJ8qVL32YAQ1LbabLIIYG8+eHdmA1GvzGaKxh3yc7i2D2W3nzyfOP w00lYWkXXV92gnWEkKnpL6bg8JisDMl2PYgbo9hdT1J34XcyEDJBMmPXZ9nDxsAb 1lf6NFO19hIKVPhrlcM0qB6lrzMpEcrqKZ8fOG2yeUI47FW4WjDmAFWFNCm1wznF yPvStJVe3EvXRZMu4HxXSwqiqW7Oel99T/PD9w88varcoCck6T9BmIozLZMBLHIM annXCUChPAF/Ft51KW1/8dwXcJkW5z2xOh+EQ2mqRmWpF6OgQCQIeYLlYn+zQpeB /PfJ/mfg0OVE0bnluQkzyNSNoHvk+CFvNeOHvOZOoy82PEvYyAuQm773xJO02KYn ox6g8MR7czZeYuguXmgwR4h8bb8SAmE1xoWl6vxY26CKm9w8lrc5iovOgHgdVsLe 5YGduQb3hNd9m+oz1DOTEpiFVqt9CtC+TtYz1PJFTe07kz9o1pIPEKjJhxovbOrD HZfyD+6kz8Y1do7Tmg3G =iC9/ -----END PGP SIGNATURE----- --Cgrdyab2wu3Akvjd--