On Mon, Mar 20, 2023 at 15:32 PM Peter Smith <smithpb2...@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