On Mon, Mar 20, 2023 at 15:32 PM Peter Smith <smithpb2250@gmail.com> wrote:
> Here are some review comments for v17-0001.
Thanks for your comments.
> ======
> src/backend/catalog/pg_publication.c
>
> 1. filter_partitions
>
> -static List *
> -filter_partitions(List *relids)
> +static void
> +filter_partitions(List *table_infos)
> {
> - List *result = NIL;
> ListCell *lc;
> - ListCell *lc2;
>
> - foreach(lc, relids)
> + foreach(lc, table_infos)
> {
> - bool skip = false;
> - List *ancestors = NIL;
> - Oid relid = lfirst_oid(lc);
> + bool skip = false;
> + List *ancestors = NIL;
> + ListCell *lc2;
> + published_rel *table_info = (published_rel *) lfirst(lc);
>
> - if (get_rel_relispartition(relid))
> - ancestors = get_partition_ancestors(relid);
> + if (get_rel_relispartition(table_info->relid))
> + ancestors = get_partition_ancestors(table_info->relid);
>
> foreach(lc2, ancestors)
> {
> Oid ancestor = lfirst_oid(lc2);
> + ListCell *lc3;
>
> /* Check if the parent table exists in the published table list. */
> - if (list_member_oid(relids, ancestor))
> + foreach(lc3, table_infos)
> {
> - skip = true;
> - break;
> + Oid relid = ((published_rel *) lfirst(lc3))->relid;
> +
> + if (relid == ancestor)
> + {
> + skip = true;
> + break;
> + }
> }
> +
> + if (skip)
> + break;
> }
>
> - if (!skip)
> - result = lappend_oid(result, relid);
> + if (skip)
> + table_infos = foreach_delete_current(table_infos, lc);
> }
> -
> - return result;
> }
>
>
> It seems the 'skip' and 'ancestors' and 'lc2' vars are not needed
> except when "if (get_rel_relispartition(table_info->relid))" is true,
> so won't it be better to restructure the code to put everything inside
> that condition. Then you will save a few unnecessary tests of
> foreach(lc2, ancestors) and (skip).
>
> For example,
>
> static void
> filter_partitions(List *table_infos)
> {
> ListCell *lc;
>
> foreach(lc, table_infos)
> {
> published_rel *table_info = (published_rel *) lfirst(lc);
>
> if (get_rel_relispartition(table_info->relid))
> {
> bool skip = false;
> List *ancestors = get_partition_ancestors(table_info->relid);
> ListCell *lc2;
>
> foreach(lc2, ancestors)
> {
> Oid ancestor = lfirst_oid(lc2);
> ListCell *lc3;
> /* Check if the parent table exists in the published table list. */
> foreach(lc3, table_infos)
> {
> Oid relid = ((published_rel *) lfirst(lc3))->relid;
>
> if (relid == ancestor)
> {
> skip = true;
> break;
> }
> }
> if (skip)
> break;
> }
>
> if (skip)
> table_infos = foreach_delete_current(table_infos, lc);
> }
> }
> }
Refactored this part of code based on other comments.
> ~~~
>
> 3. pg_get_publication_tables
>
> /* Show all columns when the column list is not specified. */
> - if (nulls[1] == true)
> + if (nulls[2] == true)
>
> Since you are changing this line anyway, you might as well change it
> to remove the redundant "== true" part.
>
> SUGGESTION
> if (nulls[2])
Changed.
Regards,
Wang Wei
