Hi Stephen,
On 2017/04/26 0:42, Stephen Frost wrote:
> Amit,
>
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> I think why getPartitions() is separate from getInherits() and then
>> flagPartitions() separate from flagInhTables() is because I thought
>> originally that mixing the two would be undesirable. In the partitioning
>> case, getPartitions() joins pg_inherits with pg_class so that it can also
>> get hold of relpartbound, which I then thought would be a bad idea to do
>> for all of the inheritance tables. If we're OK with always doing the
>> join, I don't see why we couldn't get rid of the separation.
>
> I'm not sure what you mean here. We're always going to call both
> getInherits() and getPartitions() and run the queries in each, with the
> way the code is written today. In my experience working with pg_dump
> and trying to not slow it down, the last thing we want to do is run two
> independent queries when one will do. Further, we aren't really
> avoiding any work when we have to go look at pg_class to exclude the
> partitioned tables in getInherits() anyway. I'm not seeing why we
> really need the join at all though, see below.
You're right and I realize that we don't need lots of new code for pg_dump
to retrieve the partitioning information (including the parent-child
relationships). I propose some patches below, one per each item you
discovered could be improved.
>> flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
>> it for both inheritance and partitioning is fine. It affects NOT NULL
>> constraints and defaults, which as far as it goes, applies to both
>> inheritance and partitioning the same.
>
> I don't see why we need to have getPartitions(), flagPartitions(), or
> findPartitonParentByOid(). They all seem to be largely copies of the
> inheritance functions, but it doesn't seem like that's really necessary
> or sensible.
>
> I also don't follow why we're pulling the partbound definition in
> getPartitions() instead of in getTables(), where we're already pulling
> the rest of the data from pg_class? Or why getTablePartitionKeyInfo()
> exists instead of also doing that during getTables()?
I think it had to do with wanting to avoid creating yet another copy of
the big getTables() query for >= v10, back when the original patch was
written, but doing that is not really needed.
Attached patch 0004 gets rid of that separation. It removes the struct
PartInfo, functions flagPartitions(), findPartitionParentByOid(),
getPartitions(), and getTablePartitionKeyInfo(). All necessary
partitioning-specific information is retrieved in getTables() itself.
Also, now that partitioning uses the old inheritance code, inherited NOT
NULL constraints need not be emitted separately per child. The
now-removed code that separately retrieved partitioning inheritance
information was implemented such that partition attributes didn't receive
the flagInhAttrs() treatment.
> I looked through
> pg_get_partkeydef() and it didn't seem to be particularly expensive to
> run, though evidently it doesn't handle being passed an OID that it
> doesn't expect very cleanly:
>
> =# select pg_get_partkeydef(oid) from pg_class;
> ERROR: cache lookup failed for partition key of 52333
>
> I don't believe that's really very appropriate of it to do though and
> instead it should work the same way pg_get_indexdef() and others do and
> return NULL in such cases, so people can use it sanely.
>
> I'm working through this but it's going to take a bit of time to clean
> it up and it's not going to get finished today, but I do think it needs
> to get done and so I'll work to make it happen this week. I didn't
> anticipate finding this, obviously and am a bit disappointed by it.
Yeah, that was sloppy. :-(
Attached patch 0005 fixes that and adds a test to rules.sql.
>> So, the newly added tests seem enough for now?
>
> Considering the pg_upgrade regression test didn't pass with the patch
> as sent, I'd say that more tests are needed. I will be working to add
> those also.
I think I have found an oversight in the pg_dump's --binary-upgrade code
that might have caused the error you saw (I proceeded to fix it after
seeing the error that I saw). Fix is included as patch 0003.
So to summarize what the patches do (some of these were posted earlier)
0001: Improve test coverage of partition constraints (non-inherited ones)
0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
INHERIT to be emitted to attach a partition instead of ALTER TABLE
ATTACH PARTITION
0004: Change the way pg_dump retrieves partitioning info (removes a lot
of unnecessary code and instead makes partitioning case use the old
inheritance code, etc.)
0005: Teach pg_get_partkeydef_worker to deal with OIDs it can't handle
Thanks,
Amit
>From 5ba1b532b020648457e92d34d5f2712f36efd9ff Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 18 Apr 2017 10:59:35 +0900
Subject: [PATCH 1/5] 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 7f0b7768352cc7031f5b482b2c9e1526d036ffe2 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 12 Apr 2017 15:16:56 +0900
Subject: [PATCH 2/5] Do not emit WITH OPTIONS for partition's columns
CREATE TABLE OF requires it, but CREATE TABLE PARTITION OF doesn't.
---
src/bin/pg_dump/pg_dump.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e9b5c8a448..5016c2de74 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15271,7 +15271,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if ((tbinfo->reloftype || tbinfo->partitionOf) &&
!dopt->binary_upgrade)
{
- appendPQExpBufferStr(q, " WITH OPTIONS");
+ if (tbinfo->reloftype)
+ appendPQExpBufferStr(q, " WITH OPTIONS");
}
else
{
--
2.11.0
>From 395a814df9641ed2b19bd8fa5a97e5ce1004a5f1 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 26 Apr 2017 16:03:20 +0900
Subject: [PATCH 3/5] 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 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5016c2de74..5f3ec51011 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15488,7 +15488,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
}
- if (numParents > 0)
+ /* Note that partitions are handled differently (see below) */
+ if (numParents > 0 && !tbinfo->partitionOf)
{
appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance this way.\n");
for (k = 0; k < numParents; k++)
--
2.11.0
>From b4b94fdd94fd0814239c077fbbee6a7168f6ae7e Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 26 Apr 2017 14:18:21 +0900
Subject: [PATCH 4/5] 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 | 191 +++++++++++++--------------------------
src/bin/pg_dump/pg_dump.h | 15 +--
src/bin/pg_dump/t/002_pg_dump.pl | 2 -
4 files changed, 65 insertions(+), 233 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 5f3ec51011..942c8f522e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -5507,6 +5507,27 @@ 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 = "CASE WHEN c.relkind = 'p' THEN"
+ " pg_catalog.pg_get_partkeydef(c.oid)"
+ " ELSE NULL END";
+ ispartition = "c.relispartition";
+ partbound = "pg_get_expr(c.relpartbound, c.oid)";
+ }
/* Make sure we are in proper schema */
selectSourceSchema(fout, "pg_catalog");
@@ -5594,7 +5615,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 +5642,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 +6065,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 +6170,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 +6300,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 +6327,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 +7695,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 +15118,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 +15166,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 +15194,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)
{
if (tbinfo->reloftype)
@@ -15328,7 +15260,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))
{
/*
@@ -15338,13 +15270,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++)
@@ -15489,7 +15424,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
}
/* Note that partitions are handled differently (see below) */
- if (numParents > 0 && !tbinfo->partitionOf)
+ if (numParents > 0 && !tbinfo->ispartition)
{
appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance this way.\n");
for (k = 0; k < numParents; k++)
@@ -15514,14 +15449,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 bb1ac8ea480266b2cff334001197a393137b9eff Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 26 Apr 2017 14:50:49 +0900
Subject: [PATCH 5/5] Teach pg_get_partkeydef_worker to deal with OIDs it can't
handle
---
src/backend/utils/adt/ruleutils.c | 20 ++++++++++++++------
src/test/regress/expected/rules.out | 6 ++++++
src/test/regress/sql/rules.sql | 1 +
3 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 184e5daa05..cbde1fff01 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -320,7 +320,7 @@ static char *pg_get_indexdef_worker(Oid indexrelid, int colno,
int prettyFlags, bool missing_ok);
static char *pg_get_statisticsext_worker(Oid statextid, bool missing_ok);
static char *pg_get_partkeydef_worker(Oid relid, int prettyFlags,
- bool attrsOnly);
+ bool attrsOnly, bool missing_ok);
static char *pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
int prettyFlags, bool missing_ok);
static text *pg_get_expr_worker(text *expr, Oid relid, const char *relname,
@@ -1555,10 +1555,14 @@ Datum
pg_get_partkeydef(PG_FUNCTION_ARGS)
{
Oid relid = PG_GETARG_OID(0);
+ char *res;
+
+ res = pg_get_partkeydef_worker(relid, PRETTYFLAG_INDENT, false, true);
+
+ if (res == NULL)
+ PG_RETURN_NULL();
- PG_RETURN_TEXT_P(string_to_text(pg_get_partkeydef_worker(relid,
- PRETTYFLAG_INDENT,
- false)));
+ PG_RETURN_TEXT_P(string_to_text(res));
}
/* Internal version that just reports the column definitions */
@@ -1568,7 +1572,7 @@ pg_get_partkeydef_columns(Oid relid, bool pretty)
int prettyFlags;
prettyFlags = pretty ? PRETTYFLAG_PAREN | PRETTYFLAG_INDENT : PRETTYFLAG_INDENT;
- return pg_get_partkeydef_worker(relid, prettyFlags, true);
+ return pg_get_partkeydef_worker(relid, prettyFlags, true, false);
}
/*
@@ -1576,7 +1580,7 @@ pg_get_partkeydef_columns(Oid relid, bool pretty)
*/
static char *
pg_get_partkeydef_worker(Oid relid, int prettyFlags,
- bool attrsOnly)
+ bool attrsOnly, bool missing_ok)
{
Form_pg_partitioned_table form;
HeapTuple tuple;
@@ -1594,7 +1598,11 @@ pg_get_partkeydef_worker(Oid relid, int prettyFlags,
tuple = SearchSysCache1(PARTRELID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
+ {
+ if (missing_ok)
+ return NULL;
elog(ERROR, "cache lookup failed for partition key of %u", relid);
+ }
form = (Form_pg_partitioned_table) GETSTRUCT(tuple);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 409692d695..0165471a4d 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3212,6 +3212,12 @@ SELECT pg_get_function_arg_default('pg_class'::regclass, 0);
(1 row)
+SELECT pg_get_partkeydef(0);
+ pg_get_partkeydef
+-------------------
+
+(1 row)
+
-- test rename for a rule defined on a partitioned table
CREATE TABLE parted_table (a int) PARTITION BY LIST (a);
CREATE TABLE parted_table_1 PARTITION OF parted_table FOR VALUES IN (1);
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 4fff266216..98a8a692d8 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1163,6 +1163,7 @@ SELECT pg_get_function_identity_arguments(0);
SELECT pg_get_function_result(0);
SELECT pg_get_function_arg_default(0, 0);
SELECT pg_get_function_arg_default('pg_class'::regclass, 0);
+SELECT pg_get_partkeydef(0);
-- test rename for a rule defined on a partitioned table
CREATE TABLE parted_table (a int) PARTITION BY LIST (a);
--
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
