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 <sfr...@snowman.net>
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 ] <replaceable class="PARAMETER">table_name
CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable class="PARAMETER">table_name</replaceable>
PARTITION OF <replaceable class="PARAMETER">parent_table</replaceable> [ (
- { <replaceable class="PARAMETER">column_name</replaceable> WITH OPTIONS [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
+ { <replaceable class="PARAMETER">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable> }
[, ... ]
) ] <replaceable class="PARAMETER">partition_bound_spec</replaceable>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.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 } | UNLOGGED ] TABLE [ IF NOT EXI
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="PARAMETER">table_name</replaceable>
OF <replaceable class="PARAMETER">type_name</replaceable> [ (
- { <replaceable class="PARAMETER">column_name</replaceable> WITH OPTIONS [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
+ { <replaceable class="PARAMETER">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable> }
[, ... ]
) ]
@@ -46,7 +46,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="PARAMETER">table_name</replaceable>
PARTITION OF <replaceable class="PARAMETER">parent_table</replaceable> [ (
- { <replaceable class="PARAMETER">column_name</replaceable> [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
+ { <replaceable class="PARAMETER">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable> }
[, ... ]
) ] FOR VALUES <replaceable class="PARAMETER">partition_bound_spec</replaceable>
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, List *aliases, Node *query);
%type <str> part_strategy
%type <partelem> part_elem
%type <list> part_params
-%type <list> OptPartitionElementList PartitionElementList
-%type <node> PartitionElement
%type <node> ForValues
%type <node> partbound_datum
%type <list> partbound_datum_list
@@ -3131,7 +3129,7 @@ CreateStmt: CREATE OptTemp TABLE qualified_name '(' OptTableElementList ')'
$$ = (Node *)n;
}
| CREATE OptTemp TABLE qualified_name PARTITION OF qualified_name
- OptPartitionElementList ForValues OptPartitionSpec OptWith
+ OptTypedTableElementList ForValues OptPartitionSpec OptWith
OnCommitOption OptTableSpace
{
CreateStmt *n = makeNode(CreateStmt);
@@ -3150,7 +3148,7 @@ CreateStmt: CREATE OptTemp TABLE qualified_name '(' OptTableElementList ')'
$$ = (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 = makeNode(CreateStmt);
@@ -3213,11 +3211,6 @@ OptTypedTableElementList:
| /*EMPTY*/ { $$ = NIL; }
;
-OptPartitionElementList:
- '(' PartitionElementList ')' { $$ = $2; }
- | /*EMPTY*/ { $$ = NIL; }
- ;
-
TableElementList:
TableElement
{
@@ -3240,17 +3233,6 @@ TypedTableElementList:
}
;
-PartitionElementList:
- PartitionElement
- {
- $$ = list_make1($1);
- }
- | PartitionElementList ',' PartitionElement
- {
- $$ = lappend($1, $3);
- }
- ;
-
TableElement:
columnDef { $$ = $1; }
| TableLikeClause { $$ = $1; }
@@ -3262,28 +3244,6 @@ TypedTableElement:
| TableConstraint { $$ = $1; }
;
-PartitionElement:
- TableConstraint { $$ = $1; }
- | ColId ColQualList
- {
- ColumnDef *n = makeNode(ColumnDef);
- n->colname = $1;
- n->typeName = NULL;
- n->inhcount = 0;
- n->is_local = true;
- n->is_not_null = false;
- n->is_from_type = false;
- n->storage = 0;
- n->raw_default = NULL;
- n->cooked_default = NULL;
- n->collOid = InvalidOid;
- SplitColQualList($2, &n->constraints, &n->collClause,
- yyscanner);
- n->location = @1;
- $$ = (Node *) n;
- }
- ;
-
columnDef: ColId Typename create_generic_options ColQualList
{
ColumnDef *n = makeNode(ColumnDef);
@@ -3305,7 +3265,25 @@ columnDef: ColId Typename create_generic_options ColQualList
}
;
-columnOptions: ColId WITH OPTIONS ColQualList
+columnOptions: ColId ColQualList
+ {
+ ColumnDef *n = makeNode(ColumnDef);
+ n->colname = $1;
+ n->typeName = NULL;
+ n->inhcount = 0;
+ n->is_local = true;
+ n->is_not_null = false;
+ n->is_from_type = false;
+ n->storage = 0;
+ n->raw_default = NULL;
+ n->cooked_default = NULL;
+ n->collOid = InvalidOid;
+ SplitColQualList($2, &n->constraints, &n->collClause,
+ yyscanner);
+ n->location = @1;
+ $$ = (Node *)n;
+ }
+ | ColId WITH OPTIONS ColQualList
{
ColumnDef *n = makeNode(ColumnDef);
n->colname = $1;
@@ -4872,7 +4850,7 @@ CreateForeignTableStmt:
$$ = (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 = makeNode(CreateForeignTableStmt);
@@ -4893,7 +4871,7 @@ CreateForeignTableStmt:
$$ = (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 = 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;
}
- /* 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->partitionOf))
{
appendPQExpBuffer(q, " %s",
tbinfo->atttypnames[j]);
--
2.7.4
signature.asc
Description: Digital signature
