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 <amitlangot...@gmail.com> 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 <amitlangot...@gmail.com> 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 <amitlangot...@gmail.com> 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 <amitlangot...@gmail.com> 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 (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers