Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Hi,
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.
In this case we might need to explore all partitions and exclude or include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?
Regards,
Amul
Attachments:
pg_dump_fix_for_partitioned_rel_wip.patchapplication/octet-stream; name=pg_dump_fix_for_partitioned_rel_wip.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index af84c25..38b25b0 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1249,6 +1249,49 @@ expand_schema_name_patterns(Archive *fout,
destroyPQExpBuffer(query);
}
+/* TODO: comments? */
+static void
+find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids)
+{
+
+ PGresult *partitions;
+ int partition;
+ PQExpBuffer query = createPQExpBuffer();
+
+ appendPQExpBuffer(query, "SELECT inhrelid "
+ "FROM pg_catalog.pg_inherits "
+ "WHERE inhparent = %u", parentId);
+
+ partitions = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ for (partition = 0; partition < PQntuples(partitions); partition++)
+ {
+ PGresult *res;
+ char *relkind;
+ Oid partid = atooid(PQgetvalue(partitions, partition, 0));
+
+ simple_oid_list_append(oids, partid);
+
+ /* TODO: comments? */
+ resetPQExpBuffer(query);
+ appendPQExpBuffer(query, "SELECT relkind "
+ "FROM pg_catalog.pg_class "
+ "WHERE oid = %u", partid);
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ if (PQntuples(res) == 0)
+ exit_horribly(NULL, "no matching partition tables were ");
+
+ relkind = PQgetvalue(res, 0, 0);
+
+ if (relkind[0] == RELKIND_PARTITIONED_TABLE)
+ find_partition_by_relid(fout, partid, oids);
+ }
+ PQclear(partitions);
+ destroyPQExpBuffer(query);
+}
+
/*
* Find the OIDs of all tables matching the given list of patterns,
* and append them to the given OID list.
@@ -1276,7 +1319,7 @@ expand_table_name_patterns(Archive *fout,
for (cell = patterns->head; cell; cell = cell->next)
{
appendPQExpBuffer(query,
- "SELECT c.oid"
+ "SELECT c.oid, c.relkind"
"\nFROM pg_catalog.pg_class c"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace"
"\nWHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c')\n",
@@ -1293,7 +1336,17 @@ expand_table_name_patterns(Archive *fout,
for (i = 0; i < PQntuples(res); i++)
{
- simple_oid_list_append(oids, atooid(PQgetvalue(res, i, 0)));
+ Oid relOid = atooid(PQgetvalue(res, i, 0));
+ simple_oid_list_append(oids, relOid);
+
+ /* TODO: comments? */
+ if (fout->remoteVersion >= 100000)
+ {
+ char *relkind = PQgetvalue(res, i, 1);
+
+ if (relkind[0] == RELKIND_PARTITIONED_TABLE)
+ find_partition_by_relid(fout, relOid, oids);
+ }
}
PQclear(res);
Hi,
On Tue, May 9, 2017 at 12:43 PM, amul sul <sulamul@gmail.com> wrote:
Hi,
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.In this case we might need to explore all partitions and exclude or include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?Regards,
Amul
+1.
Also, I can see similar issue exists with inheritance.
In attached patch, I have extended Amul's original patch to address the
inheritance dumping issue.
Regards,
Jeevan Ladhe
Attachments:
pg_dump_fix_for_partition_and_inheritance.patchapplication/octet-stream; name=pg_dump_fix_for_partition_and_inheritance.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index af84c25..38b25b0 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1249,6 +1249,49 @@ expand_schema_name_patterns(Archive *fout,
destroyPQExpBuffer(query);
}
+/* TODO: comments? */
+static void
+find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids)
+{
+
+ PGresult *partitions;
+ int partition;
+ PQExpBuffer query = createPQExpBuffer();
+
+ appendPQExpBuffer(query, "SELECT inhrelid "
+ "FROM pg_catalog.pg_inherits "
+ "WHERE inhparent = %u", parentId);
+
+ partitions = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ for (partition = 0; partition < PQntuples(partitions); partition++)
+ {
+ PGresult *res;
+ char *relkind;
+ Oid partid = atooid(PQgetvalue(partitions, partition, 0));
+
+ simple_oid_list_append(oids, partid);
+
+ /* TODO: comments? */
+ resetPQExpBuffer(query);
+ appendPQExpBuffer(query, "SELECT relkind "
+ "FROM pg_catalog.pg_class "
+ "WHERE oid = %u", partid);
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ if (PQntuples(res) == 0)
+ exit_horribly(NULL, "no matching partition tables were ");
+
+ relkind = PQgetvalue(res, 0, 0);
+
+ if (relkind[0] == RELKIND_PARTITIONED_TABLE)
+ find_partition_by_relid(fout, partid, oids);
+ }
+ PQclear(partitions);
+ destroyPQExpBuffer(query);
+}
+
/*
* Find the OIDs of all tables matching the given list of patterns,
* and append them to the given OID list.
@@ -1276,7 +1319,7 @@ expand_table_name_patterns(Archive *fout,
for (cell = patterns->head; cell; cell = cell->next)
{
appendPQExpBuffer(query,
- "SELECT c.oid"
+ "SELECT c.oid, c.relkind"
"\nFROM pg_catalog.pg_class c"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace"
"\nWHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c')\n",
@@ -1293,7 +1336,17 @@ expand_table_name_patterns(Archive *fout,
for (i = 0; i < PQntuples(res); i++)
{
- simple_oid_list_append(oids, atooid(PQgetvalue(res, i, 0)));
+ Oid relOid = atooid(PQgetvalue(res, i, 0));
+ simple_oid_list_append(oids, relOid);
+
+ /* TODO: comments? */
+ if (fout->remoteVersion >= 100000)
+ {
+ char *relkind = PQgetvalue(res, i, 1);
+
+ if (relkind[0] == RELKIND_PARTITIONED_TABLE)
+ find_partition_by_relid(fout, relOid, oids);
+ }
}
PQclear(res);
In my last email by mistake I attached Amul's patch itself.
Please find attached patch extending the fix to inheritance relations.
Regards,
Jeevan Ladhe
On Tue, May 9, 2017 at 1:51 PM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
wrote:
Show quoted text
Hi,
On Tue, May 9, 2017 at 12:43 PM, amul sul <sulamul@gmail.com> wrote:
Hi,
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.In this case we might need to explore all partitions and exclude or
include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?Regards,
Amul+1.
Also, I can see similar issue exists with inheritance.
In attached patch, I have extended Amul's original patch to address the
inheritance dumping issue.Regards,
Jeevan Ladhe
Attachments:
pg_dump_fix_for_partition_and_inheritance_wip.patchapplication/octet-stream; name=pg_dump_fix_for_partition_and_inheritance_wip.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index af84c25..d7930d4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1249,6 +1249,49 @@ expand_schema_name_patterns(Archive *fout,
destroyPQExpBuffer(query);
}
+/* TODO: comments? */
+static void
+find_childrels_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids)
+{
+
+ PGresult *partitions;
+ int partition;
+ PQExpBuffer query = createPQExpBuffer();
+
+ appendPQExpBuffer(query, "SELECT inhrelid "
+ "FROM pg_catalog.pg_inherits "
+ "WHERE inhparent = %u", parentId);
+
+ partitions = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ for (partition = 0; partition < PQntuples(partitions); partition++)
+ {
+ PGresult *res;
+ char *relkind;
+ Oid partid = atooid(PQgetvalue(partitions, partition, 0));
+
+ simple_oid_list_append(oids, partid);
+
+ /* TODO: comments? */
+ resetPQExpBuffer(query);
+ appendPQExpBuffer(query, "SELECT relkind "
+ "FROM pg_catalog.pg_class "
+ "WHERE oid = %u", partid);
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ if (PQntuples(res) == 0)
+ exit_horribly(NULL, "no matching partition tables were ");
+
+ relkind = PQgetvalue(res, 0, 0);
+
+ if (relkind[0] == RELKIND_PARTITIONED_TABLE)
+ find_partition_by_relid(fout, partid, oids);
+ }
+ PQclear(partitions);
+ destroyPQExpBuffer(query);
+}
+
/*
* Find the OIDs of all tables matching the given list of patterns,
* and append them to the given OID list.
@@ -1276,7 +1319,7 @@ expand_table_name_patterns(Archive *fout,
for (cell = patterns->head; cell; cell = cell->next)
{
appendPQExpBuffer(query,
- "SELECT c.oid"
+ "SELECT c.oid, c.relkind, c.relhassubclass"
"\nFROM pg_catalog.pg_class c"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace"
"\nWHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c')\n",
@@ -1293,7 +1336,18 @@ expand_table_name_patterns(Archive *fout,
for (i = 0; i < PQntuples(res); i++)
{
- simple_oid_list_append(oids, atooid(PQgetvalue(res, i, 0)));
+ Oid relOid = atooid(PQgetvalue(res, i, 0));
+ simple_oid_list_append(oids, relOid);
+
+ /* TODO: comments? */
+ if (fout->remoteVersion >= 100000)
+ {
+ char *relkind = PQgetvalue(res, i, 1);
+ char *relhassubclass = PQgetvalue(res, i, 2);
+
+ if (relkind[0] == RELKIND_PARTITIONED_TABLE || relhassubclass[0] == 't')
+ find_childrel_by_relid(fout, relOid, oids);
+ }
}
PQclear(res);
Hi Amul,
On 2017/05/09 16:13, amul sul wrote:
Hi,
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.
I agree that `pg_dump -t partitioned_table` should dump all of its
partitions too and that `pg_dump -T partitioned_table` should exclude
partitions. Your patch achieves that. Some comments:
I think we should update doc/src/sgml/ref/pg_dump.sgml to mention this
behavior.
+static void
+find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids)Is expand_partitioned_table() a slightly better name?
+ appendPQExpBuffer(query, "SELECT relkind "
+ "FROM pg_catalog.pg_class "
+ "WHERE oid = %u", partid);
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ if (PQntuples(res) == 0)
+ exit_horribly(NULL, "no matching partition tables were ");
+
+ relkind = PQgetvalue(res, 0, 0);
+
+ if (relkind[0] == RELKIND_PARTITIONED_TABLE)
+ find_partition_by_relid(fout, partid, oids);Instead of recursing like this requiring to send one query for every
partition, why not issue just one query such that all the partitions in
the inheritance tree are returned. For example, as follows:
+ appendPQExpBuffer(query, "WITH RECURSIVE partitions (partoid) AS ("
+ " SELECT inhrelid"
+ " FROM pg_inherits"
+ " WHERE inhparent = %u"
+ " UNION ALL"
+ " SELECT inhrelid"
+ " FROM pg_inherits, partitions"
+ " WHERE inhparent = partitions.partoid )"
+ " SELECT partoid FROM partitions",
+ parentId);I included your patch with the above modifications in the attached 0001
patch, which also contains the documentation updates. Please take a look.
When testing the patch, I found that if --table specifies the parent and
--exclude specifies one of its partitions, the latter won't be forcefully
be included due to the partitioned table expansion, which seems like an
acceptable behavior. On the other hand, if --table specifies a partition
and --exclude specifies the partition's parent, the latter makes partition
to be excluded as well as part of the expansion, which seems fine too.
One more thing I am wondering about is whether `pg_dump -t partition`
should be dumped as a partition definition (that is, as CREATE TABLE
PARTITION OF) which is what happens currently or as a regular table (just
CREATE TABLE)? I'm thinking the latter would be better, but would like to
confirm before writing any code for that.
I will add this as an open item. Thanks for working on it.
Thanks,
Amit
Attachments:
0001-Expand-partitioned-table-specified-using-pg_dump-s-t.patchtext/x-diff; name=0001-Expand-partitioned-table-specified-using-pg_dump-s-t.patchDownload
From 3b3103d6a23aab78592d7547e11f7e6414cfe0ec Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 9 May 2017 16:39:14 +0900
Subject: [PATCH] Expand partitioned table specified using pg_dump's -t or -T
options
Report and patch by Amul Sul.
---
doc/src/sgml/ref/pg_dump.sgml | 12 +++++----
src/bin/pg_dump/pg_dump.c | 57 +++++++++++++++++++++++++++++++++++++++++--
2 files changed, 62 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 6cf7e570ef..c90a3298ca 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -520,10 +520,11 @@ PostgreSQL documentation
<replaceable class="parameter">table</replaceable>.
For this purpose, <quote>table</> includes views, materialized views,
sequences, and foreign tables. Multiple tables
- can be selected by writing multiple <option>-t</> switches. Also, the
- <replaceable class="parameter">table</replaceable> parameter is
- interpreted as a pattern according to the same rules used by
- <application>psql</>'s <literal>\d</> commands (see <xref
+ can be selected by writing multiple <option>-t</> switches. If the
+ specified table is a partitioned table, its partitions are dumped
+ too. Also, the <replaceable class="parameter">table</replaceable>
+ parameter is interpreted as a pattern according to the same rules used
+ by <application>psql</>'s <literal>\d</> commands (see <xref
linkend="APP-PSQL-patterns" endterm="APP-PSQL-patterns-title">),
so multiple tables can also be selected by writing wildcard characters
in the pattern. When using wildcards, be careful to quote the pattern
@@ -572,7 +573,8 @@ PostgreSQL documentation
class="parameter">table</replaceable> pattern. The pattern is
interpreted according to the same rules as for <option>-t</>.
<option>-T</> can be given more than once to exclude tables
- matching any of several patterns.
+ matching any of several patterns. If the specified table is a
+ partitioned table, its partitions are not dumped either.
</para>
<para>
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index af84c25093..00146786e3 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1250,6 +1250,43 @@ expand_schema_name_patterns(Archive *fout,
}
/*
+ * Expand partitioned table and append the OIDs of its partitions to the list
+ * of tables to be dumped.
+ */
+static void
+expand_partitioned_table(Archive *fout, Oid parentId, SimpleOidList *oids)
+{
+
+ PGresult *partitions;
+ int partition;
+ PQExpBuffer query = createPQExpBuffer();
+
+ /* Get the OIDs of partitions of all levels */
+ appendPQExpBuffer(query, "WITH RECURSIVE partitions (partoid) AS ("
+ " SELECT inhrelid"
+ " FROM pg_inherits"
+ " WHERE inhparent = %u"
+ " UNION ALL"
+ " SELECT inhrelid"
+ " FROM pg_inherits, partitions"
+ " WHERE inhparent = partitions.partoid )"
+ " SELECT partoid FROM partitions",
+ parentId);
+
+ partitions = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ for (partition = 0; partition < PQntuples(partitions); partition++)
+ {
+ Oid partid = atooid(PQgetvalue(partitions, partition, 0));
+
+ simple_oid_list_append(oids, partid);
+ }
+
+ PQclear(partitions);
+ destroyPQExpBuffer(query);
+}
+
+/*
* Find the OIDs of all tables matching the given list of patterns,
* and append them to the given OID list.
*/
@@ -1276,7 +1313,7 @@ expand_table_name_patterns(Archive *fout,
for (cell = patterns->head; cell; cell = cell->next)
{
appendPQExpBuffer(query,
- "SELECT c.oid"
+ "SELECT c.oid, c.relkind"
"\nFROM pg_catalog.pg_class c"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace"
"\nWHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c')\n",
@@ -1293,7 +1330,23 @@ expand_table_name_patterns(Archive *fout,
for (i = 0; i < PQntuples(res); i++)
{
- simple_oid_list_append(oids, atooid(PQgetvalue(res, i, 0)));
+ Oid relOid = atooid(PQgetvalue(res, i, 0));
+
+ simple_oid_list_append(oids, relOid);
+
+ /*
+ * Starting in PostgreSQL 10, we have partitioned tables, whose
+ * partitions we must include in the list of tables to be included
+ * or excluded along with the partitioned table named using the
+ * command-line option.
+ */
+ if (fout->remoteVersion >= 100000)
+ {
+ char relkind = *(PQgetvalue(res, i, 1));
+
+ if (relkind == RELKIND_PARTITIONED_TABLE)
+ expand_partitioned_table(fout, relOid, oids);
+ }
}
PQclear(res);
--
2.11.0
On Tue, May 9, 2017 at 12:43 PM, amul sul <sulamul@gmail.com> wrote:
Hi,
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.In this case we might need to explore all partitions and exclude or include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?
+1 for fixing this.
Since we didn't catch this issue earlier it looks like we don't have
testcases testing this scenario. May be we should add those in the
patch.
The way this patch has been written, there is a possibility that a
partition would be included multiple times in the list of tables, if
names of the partition and the parent table both match the pattern.
That's not a correctness issue, but it would slow down
simple_oid_list_member() a bit because of longer OID list.
I am not sure what would be the use of dumping a partitioned table
without its partitions or vice-versa. I think, instead, look
partitioned table and its partitions as a single unit as far as dump
is concerned. So, either we dump partitioned table along with all its
partitions or we don't dump any of those. In that case, we should
modify the query in expand_table_name_patterns(), to not include
partitions in the result. Rest of the code in the patch would take
care of including/excluding the partitions when the parent table is
included/excluded.
This patch looks up pg_inherits for every partitioned tables that it
encounters, which is costly. Instead, a fix with better performance is
to set a table dumpable based on the corresponding setting of its
parent. The parent is available in TableInfo structure, but the
comment there says that it's set only for dumpable objects. The
comment is correct since flagInhTables() skips the tables with dump =
false. May be we should modify this function not to skip the
partitions, find their parent tables and set the dump flat based on
the parents. This means that the immediate parent's dump flag should
be set correctly before any of its children are encountered by the
flagInhTables() for the case of multi-level partitioning to work. I
don't think we can guarantee that. May be we can modify
flagInhTables() to set the parent tables of parent if that's not done
already and then set the dump flag from top parent to bottom. If we do
that, we will have to add code in tflagInhTables() to skip the entries
whose parents have been set already since those might have been set
because of an earlier grand child.
Even better if we could dump the partitions along with partitioned
table instead of creating separate TableInfo entries for those. But I
think that's a lot of change.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/05/09 17:21, Jeevan Ladhe wrote:
On Tue, May 9, 2017 at 12:43 PM, amul sul <sulamul@gmail.com> wrote:
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.In this case we might need to explore all partitions and exclude or include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?Also, I can see similar issue exists with inheritance.
In attached patch, I have extended Amul's original patch to address the
inheritance dumping issue.
Perhaps, it will be better not to touch the regular inheritance tables in
this patch.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, May 9, 2017 at 3:13 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2017/05/09 17:21, Jeevan Ladhe wrote:
On Tue, May 9, 2017 at 12:43 PM, amul sul <sulamul@gmail.com> wrote:
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.In this case we might need to explore all partitions and exclude or include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?Also, I can see similar issue exists with inheritance.
In attached patch, I have extended Amul's original patch to address the
inheritance dumping issue.Perhaps, it will be better not to touch the regular inheritance tables in
this patch.
Yeah, I think it's fine if parent gets dumped without one or more of
its children, that's user's choice when it used a certain pattern.
Problematic case might be when we dump a child without its parent and
have INHERITS clause there. pg_restore would throw an error. But in
case that problem exists it's very old and should be fixed separately.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Amit, Ashutosh,
On Tue, May 9, 2017 at 3:29 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:
On Tue, May 9, 2017 at 3:13 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/05/09 17:21, Jeevan Ladhe wrote:
On Tue, May 9, 2017 at 12:43 PM, amul sul <sulamul@gmail.com> wrote:
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option,which
I think is incorrect.
In this case we might need to explore all partitions and exclude or
include
from dump according to given pg_dump option, attaching WIP patch
proposing
same fix. Thoughts/Comments?
Also, I can see similar issue exists with inheritance.
In attached patch, I have extended Amul's original patch to address the
inheritance dumping issue.Perhaps, it will be better not to touch the regular inheritance tables in
this patch.Yeah, I think it's fine if parent gets dumped without one or more of
its children, that's user's choice when it used a certain pattern.
Problematic case might be when we dump a child without its parent and
have INHERITS clause there. pg_restore would throw an error. But in
case that problem exists it's very old and should be fixed separately.
I agree that this should be taken as a separate fix, rather than taking it
with
partition.
Regards,
Jeevan Ladhe
On Tue, May 9, 2017 at 2:59 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
Hi Amul,
On 2017/05/09 16:13, amul sul wrote:
Hi,
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.I agree that `pg_dump -t partitioned_table` should dump all of its
partitions too and that `pg_dump -T partitioned_table` should exclude
partitions. Your patch achieves that. Some comments:I think we should update doc/src/sgml/ref/pg_dump.sgml to mention this
behavior.+static void +find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids)Is expand_partitioned_table() a slightly better name?
+ appendPQExpBuffer(query, "SELECT relkind " + "FROM pg_catalog.pg_class " + "WHERE oid = %u", partid); + + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + + if (PQntuples(res) == 0) + exit_horribly(NULL, "no matching partition tables were "); + + relkind = PQgetvalue(res, 0, 0); + + if (relkind[0] == RELKIND_PARTITIONED_TABLE) + find_partition_by_relid(fout, partid, oids);Instead of recursing like this requiring to send one query for every
partition, why not issue just one query such that all the partitions in
the inheritance tree are returned. For example, as follows:+ appendPQExpBuffer(query, "WITH RECURSIVE partitions (partoid) AS (" + " SELECT inhrelid" + " FROM pg_inherits" + " WHERE inhparent = %u" + " UNION ALL" + " SELECT inhrelid" + " FROM pg_inherits, partitions" + " WHERE inhparent = partitions.partoid )" + " SELECT partoid FROM partitions", + parentId);I included your patch with the above modifications in the attached 0001
patch, which also contains the documentation updates. Please take a look.
I think this patch too has the same problem I described in my reply to
Amul; it fires a query to server for every partitioned table it
encounters, that's not very efficient.
When testing the patch, I found that if --table specifies the parent and
--exclude specifies one of its partitions, the latter won't be forcefully
be included due to the partitioned table expansion, which seems like an
acceptable behavior.
unless the partition is default partition or a hash partition.
On the other hand, if --table specifies a partition
and --exclude specifies the partition's parent, the latter makes partition
to be excluded as well as part of the expansion, which seems fine too.
I am not sure if it's worth considering partitions and partitioned
table as different entities as far as dump is concerned. We should
probably dump the whole partitioned table or none of it. There's merit
in dumping just a single partition to transfer that data to some other
server, but I am not sure how much the feature would be used.
In order to avoid any such potential anomalies, we should copy dump
flag for a partition from that of the parent as I have described in my
reply to Amul.
One more thing I am wondering about is whether `pg_dump -t partition`
should be dumped as a partition definition (that is, as CREATE TABLE
PARTITION OF) which is what happens currently or as a regular table (just
CREATE TABLE)? I'm thinking the latter would be better, but would like to
confirm before writing any code for that.
If we go about dumping a partition without its parent table, we should
dump CREATE TABLE with proper list of columns and constraints without
PARTITION OF clause. But I am not sure whether we should go that
route.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, May 9, 2017 at 3:51 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
On Tue, May 9, 2017 at 2:59 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:Hi Amul,
On 2017/05/09 16:13, amul sul wrote:
Hi,
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.I agree that `pg_dump -t partitioned_table` should dump all of its
partitions too and that `pg_dump -T partitioned_table` should exclude
partitions. Your patch achieves that. Some comments:I think we should update doc/src/sgml/ref/pg_dump.sgml to mention this
behavior.+static void +find_partition_by_relid(Archive *fout, Oid parentId, SimpleOidList *oids)Is expand_partitioned_table() a slightly better name?
+ appendPQExpBuffer(query, "SELECT relkind " + "FROM pg_catalog.pg_class " + "WHERE oid = %u", partid); + + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + + if (PQntuples(res) == 0) + exit_horribly(NULL, "no matching partition tables were "); + + relkind = PQgetvalue(res, 0, 0); + + if (relkind[0] == RELKIND_PARTITIONED_TABLE) + find_partition_by_relid(fout, partid, oids);Instead of recursing like this requiring to send one query for every
partition, why not issue just one query such that all the partitions in
the inheritance tree are returned. For example, as follows:+ appendPQExpBuffer(query, "WITH RECURSIVE partitions (partoid) AS (" + " SELECT inhrelid" + " FROM pg_inherits" + " WHERE inhparent = %u" + " UNION ALL" + " SELECT inhrelid" + " FROM pg_inherits, partitions" + " WHERE inhparent = partitions.partoid )" + " SELECT partoid FROM partitions", + parentId);I included your patch with the above modifications in the attached 0001
patch, which also contains the documentation updates. Please take a look.I think this patch too has the same problem I described in my reply to
Amul; it fires a query to server for every partitioned table it
encounters, that's not very efficient.
Agree with Ashutosh, If such information is already available then we need to
leverage of it.
When testing the patch, I found that if --table specifies the parent and
--exclude specifies one of its partitions, the latter won't be forcefully
be included due to the partitioned table expansion, which seems like an
acceptable behavior.unless the partition is default partition or a hash partition.
On the other hand, if --table specifies a partition
and --exclude specifies the partition's parent, the latter makes partition
to be excluded as well as part of the expansion, which seems fine too.I am not sure if it's worth considering partitions and partitioned
table as different entities as far as dump is concerned. We should
probably dump the whole partitioned table or none of it. There's merit
in dumping just a single partition to transfer that data to some other
server, but I am not sure how much the feature would be used.
but won't be useless.
In order to avoid any such potential anomalies, we should copy dump
flag for a partition from that of the parent as I have described in my
reply to Amul.One more thing I am wondering about is whether `pg_dump -t partition`
should be dumped as a partition definition (that is, as CREATE TABLE
PARTITION OF) which is what happens currently or as a regular table (just
CREATE TABLE)? I'm thinking the latter would be better, but would like to
confirm before writing any code for that.If we go about dumping a partition without its parent table, we should
dump CREATE TABLE with proper list of columns and constraints without
PARTITION OF clause. But I am not sure whether we should go that
route.
IMO, I think it's worth to dump CREATE TABLE without PARTITION OF clause.
Regards,
Amul
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, May 9, 2017 at 4:21 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.In this case we might need to explore all partitions and exclude or
include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?+1.
Also, I can see similar issue exists with inheritance.
That's a pretty insightful comment which makes me think that this
isn't properly categorized as a bug. Table partitioning is based on
inheritance and is intended to behave like inheritance except when
we've decided to make it behave otherwise. We made no such decision
in this case, so it behaves like inheritance in this case. So, this
is only a bug if the inheritance behavior is also a bug.
And I think there's a pretty good argument that it isn't. Are we
saying we think that it was always intended that dumping an
inheritance parent would always dump its inheritance children as well,
and the code accidentally failed to achieve that? Are we saying we'd
back-patch a behavior change in this area to correct the wrong
behavior we've had since time immemorial? I can't speak for anyone
else, but I think the first is probably false and I would vote against
the second.
That's not to say that this isn't a possibly-useful behavior change.
I can easily imagine that users would find it convenient to be able to
dump a whole partitioning hierarchy by just selecting the parent table
rather than having to list all of the partitions. But that's also
true of inheritance. Is partitioning different enough from
inheritance that the two should have different behaviors, perhaps
because the partitioning parent can't contain data but the inheritance
parent could? Or should we change the behavior for inheritance as
well, on the theory that the proposed new behavior is just plain
better than the current behavior and everyone will want it? Or should
we do nothing at all, so as to avoid breaking things for the user who
says they want to dump JUST the parent and really means it? Even if
the parent couldn't contain any rows, it's still got a schema that can
be dumped. The option of changing partitioning in one patch and then
having a separate patch later that makes a similar change for
inheritance seems like the worst option, since then users might get
any of three behaviors depending on which release they have. If we
want to change this, let's change it all at once. But first we need
to get clarity on exactly what we're changing and for what reason.
There is a question of timing. If we accept that this is not a bug
but a proposed behavior change, then it's not clear that it really
qualifies as an open item for v10. I understand that there's an urge
to keep tinkering and making things better, but it's far from clear to
me that anything bad will happen if we just postpone changing anything
until v11, especially if we decide to change both partitioning and
inheritance. I am somewhat inclined to classify this proposed open
item as a non-bug (i.e. a feature) but I'm also curious to hear what
others think.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, May 9, 2017 at 4:21 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:Also, I can see similar issue exists with inheritance.
That's a pretty insightful comment which makes me think that this
isn't properly categorized as a bug. Table partitioning is based on
inheritance and is intended to behave like inheritance except when
we've decided to make it behave otherwise. We made no such decision
in this case, so it behaves like inheritance in this case. So, this
is only a bug if the inheritance behavior is also a bug.
And I think there's a pretty good argument that it isn't.
I agree. There is room for a feature that would make --table or
--exclude-table on an inheritance parent apply to its children,
but that's a missing feature not a bug. Also, if we did make that
happen automatically, for either inheritance or partitioning,
there would inevitably be people who need to turn it off. ISTM that
the point of partitioning is mainly to be able to split up maintenance
work for a table that's too large to handle as an indivisible unit,
and it's hard to see why that wouldn't apply to dump/restore as much
as it does, say, vacuum.
So I think this can be classified as a feature to add later.
We should make sure that pg_dump behaves sanely when dumping just
some elements of a partition tree, of course. And for that matter,
pg_restore ought to be able to successfully restore just some elements
out of a an archive containing more.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 11, 2017 at 2:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, May 9, 2017 at 4:21 AM, Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:Current pg_dump --exclude-table option excludes partitioned relation
and dumps all its child relations and vice versa for --table option, which
I think is incorrect.In this case we might need to explore all partitions and exclude or
include
from dump according to given pg_dump option, attaching WIP patch proposing
same fix. Thoughts/Comments?+1.
Also, I can see similar issue exists with inheritance.
That's a pretty insightful comment which makes me think that this
isn't properly categorized as a bug. Table partitioning is based on
inheritance and is intended to behave like inheritance except when
we've decided to make it behave otherwise. We made no such decision
in this case, so it behaves like inheritance in this case. So, this
is only a bug if the inheritance behavior is also a bug.
We have chosen inheritance as mechanism to implement partitioning, but
users will have different expectations from them. Partitioned table is
a "table" "containing" partitions. So, if we want to dump a table
which is partitioned, we better dump its partitions (which happen to
be tables by themselves) as well. I don't think we can say that
inheritance parent contains its children, esp. thinking in the context
of multiple inheritance. IOW users would expect us to dump partitioned
table with its partitions and not just the shell.
In case of DROP TABLE <partitioned table> we do drop all the
partitions without specifying CASCADE. To drop an inheritance parent
we need CASCADE to drop its children. So, we have already started
diverging from inheritance.
And I think there's a pretty good argument that it isn't. Are we
saying we think that it was always intended that dumping an
inheritance parent would always dump its inheritance children as well,
and the code accidentally failed to achieve that? Are we saying we'd
back-patch a behavior change in this area to correct the wrong
behavior we've had since time immemorial? I can't speak for anyone
else, but I think the first is probably false and I would vote against
the second.
I think the inheritance behaviour we have got, whether intentional or
unintentional, is acceptable since we do not consider an inheritance
parent alongwith its children a unit, esp. when multiple inheritance
exists.
That's not to say that this isn't a possibly-useful behavior change.
I can easily imagine that users would find it convenient to be able to
dump a whole partitioning hierarchy by just selecting the parent table
rather than having to list all of the partitions. But that's also
true of inheritance.
I agree that its useful behaviour for inheritance but I am not sure
that it's a "feature" for partitioned tables.
Is partitioning different enough from
inheritance that the two should have different behaviors, perhaps
because the partitioning parent can't contain data but the inheritance
parent could?
Yes, most users would see them as different things, esp. those who
come from other DBMS backgrounds.
Or should we change the behavior for inheritance as
well, on the theory that the proposed new behavior is just plain
better than the current behavior and everyone will want it?
Not necessarily, as is the inheritance behaviour looks sane to me.
Or should
we do nothing at all, so as to avoid breaking things for the user who
says they want to dump JUST the parent and really means it? Even if
the parent couldn't contain any rows, it's still got a schema that can
be dumped. The option of changing partitioning in one patch and then
having a separate patch later that makes a similar change for
inheritance seems like the worst option, since then users might get
any of three behaviors depending on which release they have. If we
want to change this, let's change it all at once. But first we need
to get clarity on exactly what we're changing and for what reason.There is a question of timing. If we accept that this is not a bug
but a proposed behavior change, then it's not clear that it really
qualifies as an open item for v10. I understand that there's an urge
to keep tinkering and making things better, but it's far from clear to
me that anything bad will happen if we just postpone changing anything
until v11, especially if we decide to change both partitioning and
inheritance. I am somewhat inclined to classify this proposed open
item as a non-bug (i.e. a feature) but I'm also curious to hear what
others think.
To me this looks like a bug, something to be fixed in v10 only for
partitioned tables. But we don't need to entangle that with
inheritance. Partitioned tables get dumped (or not dumped) as a whole
and inheritance parents as single units.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 11, 2017 at 3:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We should make sure that pg_dump behaves sanely when dumping just
some elements of a partition tree, of course. And for that matter,
pg_restore ought to be able to successfully restore just some elements
out of a an archive containing more.
We add PARTITION OF clause for a table which is partition, so if the
parent is not present while restoring, the restore is going to fail.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:
We add PARTITION OF clause for a table which is partition, so if the
parent is not present while restoring, the restore is going to fail.
+1
But, similarly for inheritance if we dump a child table, it's dump is
broken as
of today. When we dump a child table we append "inherits(parenttab)" clause
at
the end of the DDL. Later when we try to restore this table on a database
not
having the parenttab, the restore fails.
So, I consider this as a bug.
Consider following example:
postgres=# create table tab1(a int, b int);
CREATE TABLE
postgres=# create table tab2(c int, d char) inherits(tab1);
CREATE TABLE
postgres=# \! pg_dump postgres -t tab2 > a.sql
postgres=# create database bkdb;
CREATE DATABASE
postgres=# \! psql bkdb < a.sql
SET
SET
SET
SET
SET
SET
SET
SET
SET
SET
SET
ERROR: relation "tab1" does not exist
ERROR: relation "tab2" does not exist
ERROR: relation "tab2" does not exist
invalid command \.
postgres=#
Regards,
Jeevan Ladhe
Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> writes:
On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat <
We add PARTITION OF clause for a table which is partition, so if the
parent is not present while restoring, the restore is going to fail.
+1
But, similarly for inheritance if we dump a child table, it's dump is
broken as
of today. When we dump a child table we append "inherits(parenttab)" clause
at
the end of the DDL. Later when we try to restore this table on a database
not
having the parenttab, the restore fails.
So, I consider this as a bug.
It sounds exactly what I'd expect. In particular, given that pg_dump has
worked that way for inherited tables from the beginning, it's hard to see
any must-fix bugs here.
You could argue that it would be better for pg_dump to emit something
like
CREATE TABLE c (...);
ALTER TABLE c INHERIT p;
so that if p isn't around, at least c gets created. And I think it
*would* be better, probably. But if that isn't a new feature then
I don't know what is. And partitioning really ought to track the
behavior of inheritance here.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 11, 2017 at 9:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> writes:
On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat <
We add PARTITION OF clause for a table which is partition, so if the
parent is not present while restoring, the restore is going to fail.+1
But, similarly for inheritance if we dump a child table, it's dump is
broken as
of today. When we dump a child table we append "inherits(parenttab)" clause
at
the end of the DDL. Later when we try to restore this table on a database
not
having the parenttab, the restore fails.
So, I consider this as a bug.It sounds exactly what I'd expect. In particular, given that pg_dump has
worked that way for inherited tables from the beginning, it's hard to see
any must-fix bugs here.
I agree.
You could argue that it would be better for pg_dump to emit something
likeCREATE TABLE c (...);
ALTER TABLE c INHERIT p;so that if p isn't around, at least c gets created. And I think it
*would* be better, probably. But if that isn't a new feature then
I don't know what is. And partitioning really ought to track the
behavior of inheritance here.
Hmm, I think that'd actually be worse, because it would break the use
case where you want to restore the old contents of one particular
inheritance child. So you drop that child (but not the parent or any
other children) and then do a selective restore of that one child.
Right now that works fine, but it'll fail with an error if we try to
create the already-extant parent.
I think one answer to the original complaint might be to add a new
flag to pg_dump, something like --recursive-selection, maybe -r for
short, which makes --table, --exclude-table, and --exclude-table-data
cascade to inheritance descendents. Then if you want to dump your
partition table's definition without picking up the partitions, you
can say pg_dump -t splat, and if you want the children as well you can
say pg_dump -r -t splat.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, May 11, 2017 at 9:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
You could argue that it would be better for pg_dump to emit something
likeCREATE TABLE c (...);
ALTER TABLE c INHERIT p;so that if p isn't around, at least c gets created. And I think it
*would* be better, probably. But if that isn't a new feature then
I don't know what is. And partitioning really ought to track the
behavior of inheritance here.
Hmm, I think that'd actually be worse, because it would break the use
case where you want to restore the old contents of one particular
inheritance child. So you drop that child (but not the parent or any
other children) and then do a selective restore of that one child.
Right now that works fine, but it'll fail with an error if we try to
create the already-extant parent.
Uh ... what in that is creating the already-extant parent? What
I'm envisioning is simply that the TOC entry for the child table
contains those two statements rather than "CREATE TABLE c (...)
INHERITS (p)". The equivalent thing for the partitioned case is
to use a separate ATTACH PARTITION command rather than naming the
parent immediately in the child's CREATE TABLE. This is independent
of the question of how --table and friends ought to behave.
I think one answer to the original complaint might be to add a new
flag to pg_dump, something like --recursive-selection, maybe -r for
short, which makes --table, --exclude-table, and --exclude-table-data
cascade to inheritance descendents.
Yeah, you could do it like that. Another way to do it would be to
create variants of all the selection switches, along the lines of
"--table-all=foo" meaning "foo plus its children". Then you could
have some switches recursing and others not within the same command.
But maybe that's more flexibility than needed ... and I'm having a
hard time coming up with nice switch names, anyway.
Anyway, I'm still of the opinion that it's fine to leave this as a
future feature. If we've gotten away without it this long for
inherited tables, it's unlikely to be critical for partitioned tables.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 11, 2017 at 9:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Uh ... what in that is creating the already-extant parent?
/me looks embarrassed.
Never mind. I didn't read what you wrote carefully enough.
I think one answer to the original complaint might be to add a new
flag to pg_dump, something like --recursive-selection, maybe -r for
short, which makes --table, --exclude-table, and --exclude-table-data
cascade to inheritance descendents.Yeah, you could do it like that. Another way to do it would be to
create variants of all the selection switches, along the lines of
"--table-all=foo" meaning "foo plus its children". Then you could
have some switches recursing and others not within the same command.
But maybe that's more flexibility than needed ... and I'm having a
hard time coming up with nice switch names, anyway.
I don't think that's as good. It's a lot more typing than what I
proposed and I don't think anyone is really going to want the
flexibility.
Anyway, I'm still of the opinion that it's fine to leave this as a
future feature. If we've gotten away without it this long for
inherited tables, it's unlikely to be critical for partitioned tables.
+1.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers