Re: Skipping schema changes in publication

Hi Shlok.

Some review comments for the v35-0001 patch (code)

======
src/backend/replication/pgoutput/pgoutput.c

get_rel_sync_entry:

1.
  /*
  * If this is a FOR ALL TABLES publication, pick the partition
  * root and set the ancestor level accordingly.
+ *
+ * If this is a FOR ALL TABLES publication and it has an EXCEPT
+ * TABLE list:
+ *
+ * 1. If pubviaroot is set and the relation is a partition, check
+ * whether the partition root is included in the EXCEPT TABLE
+ * list. If so, do not publish the change.
+ *
+ * 2. If pubviaroot is not set, check whether the relation itself
+ * is included in the EXCEPT TABLE list. If so, do not publish the
+ * change.
+ *
+ * This is achieved by keeping the variable "publish" set to
+ * false. And eventually, entry->pubactions will remain all false
+ * for this publication.
  */

For that last para ("This is achieved by..."), it is unclear what
"This" is referring to. I think you mean like below:

SUGGESTION
Note - "do not publish the change" is achieved by...

======
src/bin/pg_dump/pg_dump.c

getPublications:

2.
+ ntbls = PQntuples(res_tbls);
+ if (ntbls == 0)
+ continue;
+
+ for (int j = 0; j < ntbls; j++)
+ {
+ Oid prrelid;
+ TableInfo  *tbinfo;
+
+ prrelid = atooid(PQgetvalue(res_tbls, j, 0));
+
+ tbinfo = findTableByOid(prrelid);
+ if (tbinfo == NULL)
+ continue;
+
+ simple_ptr_list_append(&pubinfo[i].except_tables, tbinfo);
+ }
+
+ PQclear(res_tbls);

2a.
That first condition with 'continue' looks like it would be better
just to remove it. Otherwise, the PQclear(res_tbls) may leak. Anyway,
the loop will never iterate if ntbls is 0, so where is the harm in
removing this?

~

2b.
Also, that "if (tbinfo == NULL)" seems overkill because it is only
avoiding the final statement of the loop. It might be better to
replace this like below:

if (tblinfo != NULL)
 simple_ptr_list_append(...);

~~~

dumpPublication:

3.
+ appendPQExpBuffer(query, "ONLY %s", fmtQualifiedDumpable(tbinfo));

I think that unconditionally choosing "ONLY" here may not be the right
thing to do, particularly when the excluded table is a partitioned
table. (e.g. this is related to off-list discussions about how to
EXCEPT partition tables).

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