On Mon, Mar 20, 2023 at 21:18 PM Kuroda, Hayato/黒田 隼人 <kuroda.hay...@fujitsu.com> wrote: > Dear Wang, > > I have tested about multilevel partitions, and it worked well. > Followings are my comments for v18-0001.
Thanks for your comments and testing. > 01. pg_get_publication_tables > > ``` > + ListCell *lc; > ``` > > This definition can be inside of the "for (i = 0; i < nelems; i++)". Changed. > 02. pg_get_publication_tables > > ``` > - * If the publication publishes partition changes via > their > - * respective root partitioned tables, we must > exclude partitions > - * in favor of including the root partitioned tables. > Otherwise, > - * the function could return both the child and > parent tables > - * which could cause data of the child table to be > - * double-published on the subscriber side. > + * Publications support partitioned tables. If > + * publish_via_partition_root is false, all changes > are replicated > + * using leaf partition identity and schema, so we > only need those. > + * Otherwise, get the partitioned table itself. > ``` > > The comments can be inside of the "else". Since I think there are related operations in the function GetAllTablesPublicationRelations, it might be better to write it above the if-statement. > 03. pg_get_publication_tables > > ``` > + pfree(elems); > ``` > > Only elems is pfree()'d here, but how about other variable like pub_elem and > pub_elem_tables? Added releases to these two variables. Regards, Wang Wei
