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 have
Thanks,
Amit
>From 28cf010a8d118d115dc65e863fb8d8fd66f64305 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Tue, 18 Apr 2017 10:59:35 +0900
Subject: [PATCH 1/4] 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
+ \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
\QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN city_id SET NOT NULL;\E\n
\QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN logdate SET NOT NULL;\E\n
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index b6c75d2e81..1828f49f06 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -610,16 +610,42 @@ SELECT attname, attislocal, attinhcount FROM pg_attribute
-- able to specify column default, column constraint, and table constraint
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);
@@ -635,9 +661,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" CHECK (length(a) > 0)
- "part_b_b_check" CHECK (b >= 0)
-- Both partition bound and partition key in describe output
\d part_c
@@ -648,8 +671,6 @@ Check constraints:
b | integer | | not null | 0
Partition of: parted FOR VALUES IN ('c')
Partition key: RANGE (b)
-Check constraints:
- "check_a" CHECK (length(a) > 0)
Number of partitions: 1 (Use \d+ to list them.)
-- Show partition count in the parent's describe output
@@ -663,8 +684,6 @@ Number of partitions: 1 (Use \d+ to list them.)
a | text | | |
b | integer | | not null | 0
Partition key: LIST (a)
-Check constraints:
- "check_a" CHECK (length(a) > 0)
Number of partitions: 3 (Use \d+ to list them.)
-- cleanup
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index b00d9e87b8..ff2c7d571d 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -570,11 +570,26 @@ SELECT attname, attislocal, attinhcount FROM pg_attribute
-- able to specify column default, column constraint, and table constraint
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');
-- 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;
+
+-- 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);
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass;
+
+-- Neither check_a nor check_b are droppable from part_b
+ALTER TABLE part_b DROP CONSTRAINT check_a;
+ALTER TABLE part_b DROP CONSTRAINT check_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;
-- specify PARTITION BY for a partition
CREATE TABLE fail_part_col_not_found PARTITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
--
2.11.0
>From 09f5ea9393b34ba3df821e0545ef2e224afd6546 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Wed, 12 Apr 2017 15:16:56 +0900
Subject: [PATCH 2/4] Allow partition columns to optionally include WITH
OPTIONS keywords
---
src/backend/parser/gram.y | 20 ++++++++++++++++++++
src/test/regress/expected/create_table.out | 3 ++-
src/test/regress/sql/create_table.sql | 1 +
3 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 89d2836c49..4f674cda70 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3282,6 +3282,26 @@ PartitionElement:
n->location = @1;
$$ = (Node *) n;
}
+
+ /* Optionally, allow WITH OPTIONS keywords */
+ | ColId WITH OPTIONS 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($4, &n->constraints, &n->collClause,
+ yyscanner);
+ n->location = @1;
+ $$ = (Node *) n;
+ }
;
columnDef: ColId Typename create_generic_options ColQualList
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 1828f49f06..21173f7563 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -610,6 +610,7 @@ SELECT attname, attislocal, attinhcount FROM pg_attribute
-- able to specify column default, column constraint, and table constraint
CREATE TABLE part_b PARTITION OF parted (
+ a WITH OPTIONS NOT NULL,
b NOT NULL DEFAULT 1,
CONSTRAINT check_a CHECK (length(a) > 0),
CONSTRAINT check_b CHECK (b >= 0)
@@ -658,7 +659,7 @@ CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
Table "public.part_b"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
- a | text | | |
+ a | text | | not null |
b | integer | | not null | 1
Partition of: parted FOR VALUES IN ('b')
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index ff2c7d571d..d1c2926ad3 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -570,6 +570,7 @@ SELECT attname, attislocal, attinhcount FROM pg_attribute
-- able to specify column default, column constraint, and table constraint
CREATE TABLE part_b PARTITION OF parted (
+ a WITH OPTIONS NOT NULL,
b NOT NULL DEFAULT 1,
CONSTRAINT check_a CHECK (length(a) > 0),
CONSTRAINT check_b CHECK (b >= 0)
--
2.11.0
>From a75314ea01c3e8d2794c9947c113bacf02f7ec56 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Wed, 26 Apr 2017 14:18:21 +0900
Subject: [PATCH 3/4] Change the way pg_dump retrieves partitioning info
Since 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.
---
src/bin/pg_dump/common.c | 90 -------------------
src/bin/pg_dump/pg_dump.c | 187 +++++++++++++--------------------------
src/bin/pg_dump/pg_dump.h | 15 +---
src/bin/pg_dump/t/002_pg_dump.pl | 2 -
4 files changed, 62 insertions(+), 232 deletions(-)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index e2bc3576dc..47191be86a 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, &numInherits);
if (g_verbose)
- write_msg(NULL, "reading partition information\n");
- partinfo = getPartitions(fout, &numPartitions);
-
- if (g_verbose)
write_msg(NULL, "reading event triggers\n");
getEventTriggers(fout, &numEventTriggers);
@@ -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 == 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(&tblinfo[i], partinfo, numPartitions);
-
- /* Mark the parent as interesting for getTableAttrs */
- if (tblinfo[i].partitionOf)
- {
- tblinfo[i].partitionOf->interesting = true;
- addObjectDependency(&tblinfo[i].dobj,
- tblinfo[i].partitionOf->dobj.dumpId);
- }
- }
-}
-
/* flagInhAttrs -
* for each dumpable table in tblinfo, flag its inherited attributes
*
@@ -992,40 +936,6 @@ findParentsByOid(TableInfo *self,
}
/*
- * findPartitionParentByOid
- * find a partition's parent in tblinfo[]
- */
-static void
-findPartitionParentByOid(TableInfo *self, PartInfo *partinfo,
- int numPartitions)
-{
- Oid oid = self->dobj.catId.oid;
- int i;
-
- for (i = 0; i < numPartitions; i++)
- {
- if (partinfo[i].partrelid == oid)
- {
- TableInfo *parent;
-
- parent = findTableByOid(partinfo[i].partparent);
- if (parent == NULL)
- {
- write_msg(NULL, "failed sanity check, parent OID %u of table \"%s\" (OID %u) not found\n",
- partinfo[i].partparent,
- self->dobj.name,
- oid);
- exit_nicely(1);
- }
- self->partitionOf = parent;
-
- /* While we're at it, also save the partdef */
- self->partitiondef = partinfo[i].partdef;
- }
- }
-}
-
-/*
* parseOidArray
* parse a string of numbers delimited by spaces into a character array
*
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e9b5c8a448..a3a41d4d34 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -5507,6 +5507,25 @@ getTables(Archive *fout, int *numTables)
int i_relpages;
int i_is_identity_sequence;
int i_changed_acl;
+ int i_partkeydef;
+ int i_ispartition;
+ int i_partbound;
+ char *partkeydef;
+ char *ispartition;
+ char *partbound;
+
+ if (fout->remoteVersion < 100000)
+ {
+ partkeydef = "NULL";
+ ispartition = "NULL";
+ partbound = "NULL";
+ }
+ else
+ {
+ partkeydef = "pg_catalog.pg_get_partkeydef(c.oid)";
+ ispartition = "c.relispartition";
+ partbound = "pg_get_expr(c.relpartbound, c.oid)";
+ }
/* Make sure we are in proper schema */
selectSourceSchema(fout, "pg_catalog");
@@ -5594,7 +5613,10 @@ getTables(Archive *fout, int *numTables)
"OR %s IS NOT NULL "
"OR %s IS NOT NULL"
"))"
- "AS changed_acl "
+ "AS changed_acl, "
+ "%s AS partkeydef, "
+ "%s AS ispartition, "
+ "%s AS partbound "
"FROM pg_class c "
"LEFT JOIN pg_depend d ON "
"(c.relkind = '%c' AND "
@@ -5618,6 +5640,9 @@ getTables(Archive *fout, int *numTables)
attracl_subquery->data,
attinitacl_subquery->data,
attinitracl_subquery->data,
+ partkeydef,
+ ispartition,
+ partbound,
RELKIND_SEQUENCE,
RELKIND_RELATION, RELKIND_SEQUENCE,
RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
@@ -6038,6 +6063,9 @@ getTables(Archive *fout, int *numTables)
i_reloftype = PQfnumber(res, "reloftype");
i_is_identity_sequence = PQfnumber(res, "is_identity_sequence");
i_changed_acl = PQfnumber(res, "changed_acl");
+ i_partkeydef = PQfnumber(res, "partkeydef");
+ i_ispartition = PQfnumber(res, "ispartition");
+ i_partbound = PQfnumber(res, "partbound");
if (dopt->lockWaitTimeout)
{
@@ -6140,6 +6168,11 @@ getTables(Archive *fout, int *numTables)
tblinfo[i].is_identity_sequence = (i_is_identity_sequence >= 0 &&
strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0);
+ /* Partition key string or NULL*/
+ tblinfo[i].partkeydef = pg_strdup(PQgetvalue(res, i, i_partkeydef));
+ tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0);
+ tblinfo[i].partbound = pg_strdup(PQgetvalue(res, i, i_partbound));
+
/*
* Read-lock target tables to make sure they aren't DROPPED or altered
* in schema before we get around to dumping them.
@@ -6265,11 +6298,7 @@ getInherits(Archive *fout, int *numInherits)
* we want more information about partitions than just the parent-child
* relationship.
*/
- appendPQExpBufferStr(query,
- "SELECT inhrelid, inhparent "
- "FROM pg_inherits "
- "WHERE inhparent NOT IN (SELECT oid FROM pg_class "
- "WHERE relkind = " CppAsString2(RELKIND_PARTITIONED_TABLE) ")");
+ appendPQExpBufferStr(query, "SELECT inhrelid, inhparent FROM pg_inherits");
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
@@ -6296,72 +6325,6 @@ getInherits(Archive *fout, int *numInherits)
}
/*
- * getPartitions
- * read all the partition inheritance and partition bound information
- * from the system catalogs return them in the PartInfo* structure
- *
- * numPartitions is set to the number of pairs read in
- */
-PartInfo *
-getPartitions(Archive *fout, int *numPartitions)
-{
- PGresult *res;
- int ntups;
- int i;
- PQExpBuffer query;
- PartInfo *partinfo;
-
- int i_partrelid;
- int i_partparent;
- int i_partbound;
-
- /* Before version 10, there are no partitions */
- if (fout->remoteVersion < 100000)
- {
- *numPartitions = 0;
- return NULL;
- }
-
- query = createPQExpBuffer();
-
- /* Make sure we are in proper schema */
- selectSourceSchema(fout, "pg_catalog");
-
- /* find the inheritance and boundary information about partitions */
-
- appendPQExpBufferStr(query,
- "SELECT inhrelid as partrelid, inhparent AS partparent,"
- " pg_get_expr(relpartbound, inhrelid) AS partbound"
- " FROM pg_class c, pg_inherits"
- " WHERE c.oid = inhrelid AND c.relispartition");
-
- res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
-
- ntups = PQntuples(res);
-
- *numPartitions = ntups;
-
- partinfo = (PartInfo *) pg_malloc(ntups * sizeof(PartInfo));
-
- i_partrelid = PQfnumber(res, "partrelid");
- i_partparent = PQfnumber(res, "partparent");
- i_partbound = PQfnumber(res, "partbound");
-
- for (i = 0; i < ntups; i++)
- {
- partinfo[i].partrelid = atooid(PQgetvalue(res, i, i_partrelid));
- partinfo[i].partparent = atooid(PQgetvalue(res, i, i_partparent));
- partinfo[i].partdef = pg_strdup(PQgetvalue(res, i, i_partbound));
- }
-
- PQclear(res);
-
- destroyPQExpBuffer(query);
-
- return partinfo;
-}
-
-/*
* getIndexes
* get information about every index on a dumpable table
*
@@ -7730,49 +7693,6 @@ getTransforms(Archive *fout, int *numTransforms)
}
/*
- * getTablePartitionKeyInfo -
- * for each interesting partitioned table, read information about its
- * partition key
- *
- * modifies tblinfo
- */
-void
-getTablePartitionKeyInfo(Archive *fout, TableInfo *tblinfo, int numTables)
-{
- PQExpBuffer q;
- int i;
- PGresult *res;
-
- /* No partitioned tables before 10 */
- if (fout->remoteVersion < 100000)
- return;
-
- q = createPQExpBuffer();
-
- for (i = 0; i < numTables; i++)
- {
- TableInfo *tbinfo = &(tblinfo[i]);
-
- /* Only partitioned tables have partition key */
- if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
- continue;
-
- /* Don't bother computing anything for non-target tables, either */
- if (!tbinfo->dobj.dump)
- continue;
-
- resetPQExpBuffer(q);
- appendPQExpBuffer(q, "SELECT pg_catalog.pg_get_partkeydef('%u'::pg_catalog.oid)",
- tbinfo->dobj.catId.oid);
- res = ExecuteSqlQuery(fout, q->data, PGRES_TUPLES_OK);
- Assert(PQntuples(res) == 1);
- tbinfo->partkeydef = pg_strdup(PQgetvalue(res, 0, 0));
- }
-
- destroyPQExpBuffer(q);
-}
-
-/*
* getTableAttrs -
* for each interesting table, read info about its attributes
* (names, types, default values, CHECK constraints, etc)
@@ -15196,9 +15116,14 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (tbinfo->reloftype && !dopt->binary_upgrade)
appendPQExpBuffer(q, " OF %s", tbinfo->reloftype);
- if (tbinfo->partitionOf && !dopt->binary_upgrade)
+ /*
+ * If the table is a partition, dump it as such; except in the case
+ * of a binary upgrade, we dump the table normally and attach it to
+ * the parent afterward.
+ */
+ if (tbinfo->ispartition && !dopt->binary_upgrade)
{
- TableInfo *parentRel = tbinfo->partitionOf;
+ TableInfo *parentRel = tbinfo->parents[0];
appendPQExpBuffer(q, " PARTITION OF ");
if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
@@ -15239,7 +15164,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
* Skip column if fully defined by reloftype or the
* partition parent.
*/
- if ((tbinfo->reloftype || tbinfo->partitionOf) &&
+ if ((tbinfo->reloftype || tbinfo->ispartition) &&
!has_default && !has_notnull && !dopt->binary_upgrade)
continue;
@@ -15267,8 +15192,13 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
continue;
}
- /* Attribute type */
- if ((tbinfo->reloftype || tbinfo->partitionOf) &&
+ /*
+ * Attribute type. If this is not a binary upgrade dump,
+ * we need not dump the type for typed tables and
+ * partitions. Because in that case, we are merely
+ * dumping column's options, not defining a new column.
+ */
+ if ((tbinfo->reloftype || tbinfo->ispartition) &&
!dopt->binary_upgrade)
{
appendPQExpBufferStr(q, " WITH OPTIONS");
@@ -15327,7 +15257,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (actual_atts)
appendPQExpBufferStr(q, "\n)");
- else if (!((tbinfo->reloftype || tbinfo->partitionOf) &&
+ else if (!((tbinfo->reloftype || tbinfo->ispartition) &&
!dopt->binary_upgrade))
{
/*
@@ -15337,13 +15267,16 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
appendPQExpBufferStr(q, " (\n)");
}
- if (tbinfo->partitiondef && !dopt->binary_upgrade)
+ if (tbinfo->ispartition && !dopt->binary_upgrade)
{
appendPQExpBufferStr(q, "\n");
- appendPQExpBufferStr(q, tbinfo->partitiondef);
+ appendPQExpBufferStr(q, tbinfo->partbound);
}
- if (numParents > 0 && !dopt->binary_upgrade)
+ /* Emit the INHERITS clause unless this is a partition. */
+ if (numParents > 0 &&
+ !tbinfo->ispartition &&
+ !dopt->binary_upgrade)
{
appendPQExpBufferStr(q, "\nINHERITS (");
for (k = 0; k < numParents; k++)
@@ -15512,14 +15445,14 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
tbinfo->reloftype);
}
- if (tbinfo->partitionOf)
+ if (tbinfo->ispartition)
{
appendPQExpBufferStr(q, "\n-- For binary upgrade, set up partitions this way.\n");
appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
- fmtId(tbinfo->partitionOf->dobj.name));
+ fmtId(tbinfo->parents[0]->dobj.name));
appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
fmtId(tbinfo->dobj.name),
- tbinfo->partitiondef);
+ tbinfo->partbound);
}
appendPQExpBufferStr(q, "\n-- For binary upgrade, set heap's relfrozenxid and relminmxid\n");
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 471cfce92a..4e6c83c943 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -294,6 +294,7 @@ typedef struct _tableInfo
bool interesting; /* true if need to collect more data */
bool dummy_view; /* view's real definition must be postponed */
bool postponed_def; /* matview must be postponed into post-data */
+ bool ispartition; /* is table a partition? */
/*
* These fields are computed only if we decide the table is interesting
@@ -319,6 +320,7 @@ typedef struct _tableInfo
struct _attrDefInfo **attrdefs; /* DEFAULT expressions */
struct _constraintInfo *checkexprs; /* CHECK constraints */
char *partkeydef; /* partition key definition */
+ char *partbound; /* partition bound definition */
bool needs_override; /* has GENERATED ALWAYS AS IDENTITY */
/*
@@ -329,8 +331,6 @@ typedef struct _tableInfo
struct _tableDataInfo *dataObj; /* TableDataInfo, if dumping its data */
int numTriggers; /* number of triggers for table */
struct _triggerInfo *triggers; /* array of TriggerInfo structs */
- struct _tableInfo *partitionOf; /* TableInfo for the partition parent */
- char *partitiondef; /* partition key definition */
} TableInfo;
typedef struct _attrDefInfo
@@ -476,15 +476,6 @@ typedef struct _inhInfo
Oid inhparent; /* OID of its parent */
} InhInfo;
-/* PartInfo isn't a DumpableObject, just temporary state */
-typedef struct _partInfo
-{
- Oid partrelid; /* OID of a partition */
- Oid partparent; /* OID of its parent */
- char *partdef; /* partition bound definition */
-} PartInfo;
-
-
typedef struct _prsInfo
{
DumpableObject dobj;
@@ -691,7 +682,6 @@ extern ConvInfo *getConversions(Archive *fout, int *numConversions);
extern TableInfo *getTables(Archive *fout, int *numTables);
extern void getOwnedSeqs(Archive *fout, TableInfo tblinfo[], int numTables);
extern InhInfo *getInherits(Archive *fout, int *numInherits);
-extern PartInfo *getPartitions(Archive *fout, int *numPartitions);
extern void getIndexes(Archive *fout, TableInfo tblinfo[], int numTables);
extern void getExtendedStatistics(Archive *fout, TableInfo tblinfo[], int numTables);
extern void getConstraints(Archive *fout, TableInfo tblinfo[], int numTables);
@@ -717,7 +707,6 @@ extern void processExtensionTables(Archive *fout, ExtensionInfo extinfo[],
int numExtensions);
extern EventTriggerInfo *getEventTriggers(Archive *fout, int *numEventTriggers);
extern void getPolicies(Archive *fout, TableInfo tblinfo[], int numTables);
-extern void getTablePartitionKeyInfo(Archive *fout, TableInfo *tblinfo, int numTables);
extern void getPublications(Archive *fout);
extern void getPublicationTables(Archive *fout, TableInfo tblinfo[],
int numTables);
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index bebb2f4f5d..7a003c7286 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4768,8 +4768,6 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
\s+\QCONSTRAINT measurement_y2006m2_unitsales_check CHECK ((unitsales >= 0))\E\n
\)\n
\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
- \QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN city_id SET NOT NULL;\E\n
- \QALTER TABLE ONLY measurement_y2006m2 ALTER COLUMN logdate SET NOT NULL;\E\n
/xm,
like => {
clean => 1,
--
2.11.0
>From 47480103c7aca7e66b0f744d7f2fb9d5f9ff64cd Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Wed, 26 Apr 2017 16:03:20 +0900
Subject: [PATCH 4/4] Fix a bug in pg_dump's --binary-upgrade code
Said bug caused pg_dump to emit invalid command to attach a partition
to its parent.
---
src/bin/pg_dump/pg_dump.c | 50 +++++++++++++++++++++++++++++++++--------------
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a3a41d4d34..5e6845a495 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15427,13 +15427,43 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
{
TableInfo *parentRel = parents[k];
- appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
- fmtId(tbinfo->dobj.name));
+ /* In the partitioning case, we alter the parent */
+ if (tbinfo->ispartition)
+ appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+ fmtId(parentRel->dobj.name));
+ else
+ appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+ fmtId(tbinfo->dobj.name));
+
+ if (tbinfo->ispartition)
+ appendPQExpBuffer(q, "ATTACH PARTITION ");
+ else
+ appendPQExpBuffer(q, "INHERIT ");
+
+ /* Add schema-name to the appropriate table */
if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
- appendPQExpBuffer(q, "%s.",
+ {
+ if (tbinfo->ispartition)
+ {
+ /* Partition name */
+ appendPQExpBuffer(q, "%s.",
+ fmtId(tbinfo->dobj.namespace->dobj.name));
+ appendPQExpBuffer(q, "%s",
+ fmtId(tbinfo->dobj.name));
+ }
+ else
+ {
+ /* Inheritance parent name */
+ appendPQExpBuffer(q, "%s.",
fmtId(parentRel->dobj.namespace->dobj.name));
- appendPQExpBuffer(q, "%s;\n",
- fmtId(parentRel->dobj.name));
+ appendPQExpBuffer(q, "%s;\n",
+ fmtId(parentRel->dobj.name));
+ }
+ }
+
+ /* Partition needs specifying the bounds */
+ if (tbinfo->ispartition)
+ appendPQExpBuffer(q, " %s;\n", tbinfo->partbound);
}
}
@@ -15445,16 +15475,6 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
tbinfo->reloftype);
}
- if (tbinfo->ispartition)
- {
- appendPQExpBufferStr(q, "\n-- For binary upgrade, set up partitions this way.\n");
- appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
- fmtId(tbinfo->parents[0]->dobj.name));
- appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
- fmtId(tbinfo->dobj.name),
- tbinfo->partbound);
- }
-
appendPQExpBufferStr(q, "\n-- For binary upgrade, set heap's relfrozenxid and relminmxid\n");
appendPQExpBuffer(q, "UPDATE pg_catalog.pg_class\n"
"SET relfrozenxid = '%u', relminmxid = '%u'\n"
--
2.11.0
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
