On Mon, Mar 20, 2023 at 21:18 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@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