RE: Data is copied twice when specifying both child and parent table in publication

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