Re: Data is copied twice when specifying both child and parent table in publication - Mailing list pgsql-hackers

Here are some review comments for v17-0001.

======
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);
}
}
}

~~~

2. pg_get_publication_tables

+ else
+ {
+ List    *relids,
+    *schemarelids;
+
+ relids = GetPublicationRelations(pub_elem->oid,
+ pub_elem->pubviaroot ?
+ PUBLICATION_PART_ROOT :
+ PUBLICATION_PART_LEAF);
+ schemarelids = GetAllSchemaPublicationRelations(pub_elem->oid,
+ pub_elem->pubviaroot ?
+ PUBLICATION_PART_ROOT :
+ PUBLICATION_PART_LEAF);
+ pub_elem_tables = list_concat_unique_oid(relids, schemarelids);
+ }

2a.
Maybe 'schema_relids' would be a better name than 'schemareliids'?

~

2b.
By introducing another variable maybe you could remove some of this
duplicated code.

PublicationPartOpt root_or_leaf = pub_elem->pubviaroot ?
PUBLICATION_PART_ROOT : PUBLICATION_PART_LEAF;

~~~

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])

======
src/include/catalog/pg_proc.dat

4.
+{ oid => '6119',
+  descr => 'get information of the tables in the given publication array',

Should that be worded in a way to make it more clear that the
"publication array" is really an "array of publication names"?

------
Kind Regards,
Peter Smith.
Fujitsu Australia