One thing that bothers me a bit about this is that there's no single
code comment where this restriction it documented in full; in fact it
doesn't seem documented anywhere, only in the commit message.
I think check_foreign_tables() is a good place to add an explanatory
comment; other places can reference that. For instance, add something
like
/*
* Protect against including foreign tables that are partitions of
* partitioned tables published by the given publication. This would
* not work properly, because <!-- explain reason -->, so we disallow
* the case here and in all DDL commands that would end up creating
* such a case indirectly.
*/
Then for instance in check_publication_add_relation() and
ATExecAttachPartition() you comment would say /* if the would-be
partition is a foreign table, verify that the partitioned table is not
in a publication with publish_via_root=false. See check_foreign_tables
for details */
Also, surely we should document this restriction in the SGML docs
somewhere.
Would it be better if check_partrel_has_foreign_table() used
RelationGetPartitionDesc(omit_detached=true) instead of
find_all_inheritors()?
I'm wary of all those accesses of subscription/publication catalogs in
DDL code. Maybe I worry about nothing, but I cannot but feel that we're
missing one layer of abstraction there (including possibly some caching
on top of syscache).
I think this
castNode(RangeVar, lfirst(list_head(stmt->base.inhRelations)));
is better written
linitial_node(RangeVar, stmt->base.inhRelations);
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)
