Re: Bug in pg_dump --table and --exclude-table for declarative partition table handling.

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