Le 25/02/2023 à 16:40, Stéphane Tachoires a écrit :
Hi,
I'm not sure about the "child" -> "partition" change as it also
selects childs that are not partitions.
I'm more dubious about the --with-childs option, I'd rather have
--table-with-childs=<PATTERN> and
--exclude-table-with-childs=<PATTERN>. That will be clearer about what
is what.
I'm working on that, but have a hard time with
test pg_dump/002_pg_dump (It's brand new to me)
Stéphane
Le ven. 24 févr. 2023 à 23:50, Cary Huang <cary.hu...@highgo.ca> a écrit :
The following review has been posted through the commitfest
application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hi
the patch applies fine on current master branch and it works as
described. However, I would suggest changing the new option name
from "--with-childs" to "--with-partitions" for several reasons.
"childs" is grammatically incorrect and in the PG community, the
term "partitioned table" is normally used to denote a parent
table, and the term "partition" is used to denote the child table
under the parent table. We should use these terms to stay
consistent with the community.
Also, I would rephrase the documentation as:
Used in conjunction with
<option>-t</option>/<option>--table</option> or
<option>-T</option>/<option>--exclude-table</option> options to
include or exclude partitions of the specified tables if any.
thank you
Cary Huang
================
HighGo Software Canada
www.highgo.ca <http://www.highgo.ca>
Hi,
This is right this patch also works with inherited tables so
--with-partitions can be confusing this is why --with-childs was chosen.
But I disagree the use of --table-with-childs and
--exclude-table-with-childs because we already have the --table and
--exclude-table, and it will add lot of code where we just need a switch
to include children tables. Actually my first though was that this
behavior (dump child tables when the root table is dumped using --table)
should be the default in pg_dump but the problem is that it could break
existing scripts using pg_dump so I prefer to implement the
--with-childs options.
I think we can use --with-partitions, provided that it is clear in the
documentation that this option also works with inheritance.
Attached is a new patch v2 using the --with-partitions and the
documentation fix.
--
Gilles Darold
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 49d218905f..15ada5c8ee 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1173,6 +1173,18 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--with-partitions</option></term>
+ <listitem>
+ <para>
+ Used in conjunction with <option>-t</option>/<option>--table</option>
+ or <option>-T</option>/<option>--exclude-table</option> options to
+ include or exclude partitions of the specified tables if any.
+ This option is also valid for tables using inheritance.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-?</option></term>
<term><option>--help</option></term>
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index aba780ef4b..2e7e919391 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -200,6 +200,7 @@ typedef struct _dumpOptions
int sequence_data; /* dump sequence data even in schema-only mode */
int do_nothing;
+ bool with_partitions;
} DumpOptions;
/*
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 24ba936332..2b7a122d7e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -421,6 +421,7 @@ main(int argc, char **argv)
{"on-conflict-do-nothing", no_argument, &dopt.do_nothing, 1},
{"rows-per-insert", required_argument, NULL, 10},
{"include-foreign-data", required_argument, NULL, 11},
+ {"with-partitions", no_argument, NULL, 12},
{NULL, 0, NULL, 0}
};
@@ -631,6 +632,10 @@ main(int argc, char **argv)
optarg);
break;
+ case 12: /* dump child table too */
+ dopt.with_partitions = true;
+ break;
+
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -809,6 +814,14 @@ main(int argc, char **argv)
false);
/* non-matching exclusion patterns aren't an error */
+ /*
+ * The include child option require that there is
+ * at least one table inclusion
+ */
+ if (dopt.with_partitions && table_include_patterns.head == NULL
+ && table_exclude_patterns.head == NULL)
+ pg_fatal("option --with-partitions requires option -t/--table or -T/--exclude-table");
+
/* Expand table selection patterns into OID lists */
if (table_include_patterns.head != NULL)
{
@@ -1087,6 +1100,9 @@ help(const char *progname)
printf(_(" --use-set-session-authorization\n"
" use SET SESSION AUTHORIZATION commands instead of\n"
" ALTER OWNER commands to set ownership\n"));
+ printf(_(" --with-partitons include or exclude from a dump all child and partition\n"
+ " tables when a parent table is specified using\n"
+ " -t/--table or -T/--exclude-table\n"));
printf(_("\nConnection options:\n"));
printf(_(" -d, --dbname=DBNAME database to dump\n"));
@@ -1519,6 +1535,15 @@ expand_table_name_patterns(Archive *fout,
PQExpBufferData dbbuf;
int dotcnt;
+ /*
+ * With --include_child we look recursively to the inheritance
+ * tree to find the partitions tables of the matching include filter
+ */
+ if (fout->dopt->with_partitions)
+ {
+ appendPQExpBuffer(query, "WITH RECURSIVE partition_tree (relid) AS (\n");
+ }
+
/*
* Query must remain ABSOLUTELY devoid of unqualified names. This
* would be unnecessary given a pg_table_is_visible() variant taking a
@@ -1546,6 +1571,20 @@ expand_table_name_patterns(Archive *fout,
prohibit_crossdb_refs(GetConnection(fout), dbbuf.data, cell->val);
termPQExpBuffer(&dbbuf);
+ if (fout->dopt->with_partitions)
+ {
+ appendPQExpBuffer(query, "\n UNION ALL"
+ "\n SELECT c.oid AS relid"
+ "\n FROM partition_tree AS p"
+ "\n JOIN pg_catalog.pg_inherits AS i"
+ "\n ON (p.relid OPERATOR(pg_catalog.=) i.inhparent)"
+ "\n JOIN pg_catalog.pg_class AS c"
+ "\n ON (c.oid OPERATOR(pg_catalog.=) i.inhrelid)"
+ "\n)"
+ "\nSELECT relid FROM partition_tree");
+
+ }
+
ExecuteSqlStatement(fout, "RESET search_path");
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
PQclear(ExecuteSqlQueryForSingleRow(fout,
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 72b19ee6cd..c744ddbbd2 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -393,6 +393,17 @@ my %pgdump_runs = (
'--exclude-table=dump_test.test_table', 'postgres',
],
},
+ exclude_child_table => {
+ dump_cmd => [
+ 'pg_dump',
+ '--no-sync',
+ '--with-partitions',
+ "--file=$tempdir/exclude_child_table.sql",
+ '--exclude-table=dump_test.measurement',
+ 'postgres',
+ ],
+ },
+
exclude_test_table_data => {
dump_cmd => [
'pg_dump',
@@ -487,6 +498,18 @@ my %pgdump_runs = (
'postgres',
],
},
+ include_child_table => {
+ dump_cmd => [
+ 'pg_dump',
+ '--no-sync',
+ '--with-partitions',
+ "--file=$tempdir/include_child_table.sql",
+ '--table=dump_test.measurement',
+ '--lock-wait-timeout='
+ . (1000 * $PostgreSQL::Test::Utils::timeout_default),
+ 'postgres',
+ ],
+ },
role => {
dump_cmd => [
'pg_dump',
@@ -615,6 +638,7 @@ my %full_runs = (
compression => 1,
createdb => 1,
defaults => 1,
+ exclude_child_table => 1,
exclude_dump_test_schema => 1,
exclude_test_table => 1,
exclude_test_table_data => 1,
@@ -1010,6 +1034,10 @@ my %tests = (
role => 1,
section_pre_data => 1,
binary_upgrade => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_child_table => 1,
},
},
@@ -1099,11 +1127,16 @@ my %tests = (
'ALTER TABLE measurement OWNER TO' => {
regexp => qr/^\QALTER TABLE dump_test.measurement OWNER TO \E.+;/m,
- like =>
- { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ section_pre_data => 1,
+ include_child_table => 1,
+ },
unlike => {
exclude_dump_test_schema => 1,
no_owner => 1,
+ exclude_child_table => 1,
},
},
@@ -1114,8 +1147,12 @@ my %tests = (
%full_runs,
role => 1,
section_pre_data => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ no_owner => 1,
+ exclude_child_table => 1,
},
- unlike => { no_owner => 1, },
},
'ALTER FOREIGN TABLE foreign_table OWNER TO' => {
@@ -2867,11 +2904,16 @@ my %tests = (
\)\n
\QPARTITION BY RANGE (logdate);\E\n
/xm,
- like =>
- { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ section_pre_data => 1,
+ include_child_table => 1,
+ },
unlike => {
binary_upgrade => 1,
exclude_dump_test_schema => 1,
+ exclude_child_table => 1,
},
},
@@ -2898,6 +2940,10 @@ my %tests = (
section_pre_data => 1,
role => 1,
binary_upgrade => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_child_table => 1,
},
},
@@ -2912,10 +2958,14 @@ my %tests = (
\QEXECUTE FUNCTION dump_test.trigger_func();\E
/xm,
like => {
- %full_runs, %dump_test_schema_runs, section_post_data => 1,
+ %full_runs,
+ %dump_test_schema_runs,
+ section_post_data => 1,
+ include_child_table => 1,
},
unlike => {
exclude_dump_test_schema => 1,
+ exclude_child_table => 1,
},
},
@@ -2943,6 +2993,10 @@ my %tests = (
section_post_data => 1,
role => 1,
binary_upgrade => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_child_table => 1,
},
},
@@ -2955,6 +3009,10 @@ my %tests = (
section_post_data => 1,
role => 1,
binary_upgrade => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_child_table => 1,
},
},
@@ -2967,6 +3025,10 @@ my %tests = (
section_post_data => 1,
role => 1,
binary_upgrade => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_child_table => 1,
},
},
@@ -3324,6 +3386,7 @@ my %tests = (
schema_only => 1,
section_post_data => 1,
test_schema_plus_large_objects => 1,
+ include_child_table => 1,
},
unlike => {
exclude_dump_test_schema => 1,
@@ -3332,6 +3395,7 @@ my %tests = (
pg_dumpall_globals_clean => 1,
role => 1,
section_pre_data => 1,
+ exclude_child_table => 1,
},
},
@@ -3345,9 +3409,16 @@ my %tests = (
\QALTER TABLE ONLY dump_test.measurement\E \n^\s+
\QADD CONSTRAINT measurement_pkey PRIMARY KEY (city_id, logdate);\E
/xm,
- like =>
- { %full_runs, %dump_test_schema_runs, section_post_data => 1, },
- unlike => { exclude_dump_test_schema => 1, },
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ section_post_data => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ exclude_child_table => 1,
+ },
},
'CREATE INDEX ... ON measurement_y2006_m2' => {
@@ -3358,6 +3429,10 @@ my %tests = (
%full_runs,
role => 1,
section_post_data => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_child_table => 1,
},
},
@@ -3369,6 +3444,10 @@ my %tests = (
%full_runs,
role => 1,
section_post_data => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ exclude_child_table => 1,
},
},
@@ -3398,6 +3477,7 @@ my %tests = (
role => 1,
schema_only => 1,
section_post_data => 1,
+ include_child_table => 1,
},
unlike => {
only_dump_test_schema => 1,
@@ -3406,6 +3486,7 @@ my %tests = (
pg_dumpall_globals_clean => 1,
section_pre_data => 1,
test_schema_plus_large_objects => 1,
+ exclude_child_table => 1,
},
},
@@ -3688,11 +3769,16 @@ my %tests = (
TO regress_dump_test_role;',
regexp =>
qr/^\QGRANT SELECT ON TABLE dump_test.measurement TO regress_dump_test_role;\E/m,
- like =>
- { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ section_pre_data => 1,
+ include_child_table => 1,
+ },
unlike => {
exclude_dump_test_schema => 1,
no_privs => 1,
+ exclude_child_table => 1,
},
},
@@ -3710,8 +3796,12 @@ my %tests = (
%full_runs,
role => 1,
section_pre_data => 1,
+ include_child_table => 1,
+ },
+ unlike => {
+ no_privs => 1,
+ exclude_child_table => 1,
},
- unlike => { no_privs => 1, },
},
'GRANT ALL ON LARGE OBJECT ...' => {
@@ -3958,6 +4048,7 @@ my %tests = (
only_dump_test_table => 1,
role => 1,
section_pre_data => 1,
+ include_child_table => 1,
},
unlike => { no_privs => 1, },
},
@@ -4180,6 +4271,13 @@ $node->command_fails_like(
qr/pg_dumpall: error: improper qualified name \(too many dotted names\): myhost\.mydb/,
'pg_dumpall: option --exclude-database rejects multipart database names');
+#########################################
+# Test invalid use of --with-partitions
+$node->command_fails_like(
+ [ 'pg_dump', '-p', "$port", '--with-partitions' ],
+ qr/pg_dump: error: option --with-partitions requires option -t\/--table or -T\/--exclude-table/,
+ 'pg_dump: option --with-partitions require inclusion or exclusion of tables');
+
#########################################
# Test valid database exclusion patterns
