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)
