On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > 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 */ >
I have added comments as suggested above. > Also, surely we should document this restriction in the SGML docs > somewhere. > I have added comment in create_publication.sgml > > 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 also think using RelationGetPartitionDesc would be better and made the change. > I think this > castNode(RangeVar, lfirst(list_head(stmt->base.inhRelations))); > is better written > linitial_node(RangeVar, stmt->base.inhRelations); Fixed. I have attached the updated v10 patch with the above changes. Thanks and Regards, Shlok Kyal
v10-0001-Restrict-publishing-of-partitioned-table-with-fo.patch
Description: Binary data
