On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <alvherre@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
