Re: Skipping schema changes in publication - Mailing list pgsql-hackers

Hi Shlok

Some review comments for patch v34-0001 (code)

======
src/backend/catalog/pg_publication.c

1.
+static List *
+get_publication_relations_internal(Oid pubid, PublicationPartOpt pub_partopt,
+    bool except_flag)

No need to name this function as "_internal"; the snake_case name and
static already indicate it is internal.

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

getPublications:

2.
+ if (fout->remoteVersion >= 190000)
+ {
+ int ntbls;
+ PGresult   *res_tbls;
+
+ resetPQExpBuffer(query);
+ appendPQExpBuffer(query,
+   "SELECT prrelid\n"
+   "FROM pg_catalog.pg_publication_rel\n"
+   "WHERE prpubid = %u and prexcept = true",
+   pubinfo[i].dobj.catId.oid);
+
+ res_tbls = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+ 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.
I suppose this code is for populating the list of all tables except
those excluded, but there is no explanatory comment stating the
purpose of all this.

~

2b.
BEFORE
"WHERE prpubid = %u and prexcept = true"

SUGGESTION
"WHERE prpubid = %u AND prexcept"

~~~

dumpPublication:

3.
+ {
+ bool first_tbl = true;
+
  appendPQExpBufferStr(query, " FOR ALL TABLES");
+
+ /* Include exception tables if the publication has EXCEPT TABLEs */
+ for (SimplePtrListCell *cell = pubinfo->except_tables.head; cell;
cell = cell->next)
+ {
+ TableInfo  *tbinfo = (TableInfo *) cell->ptr;
+
+ if (first_tbl)
+ {
+ appendPQExpBufferStr(query, " EXCEPT TABLE (");
+ first_tbl = false;
+ }
+ else
+ appendPQExpBufferStr(query, ", ");
+ appendPQExpBuffer(query, "ONLY %s", fmtQualifiedDumpable(tbinfo));
+ }
+ if (!first_tbl)
+ appendPQExpBufferStr(query, ")");
+ }

3a.
That code comment seems backwards.

BEFORE
/* Include exception tables if the publication has EXCEPT TABLEs */

SUGGESTION
/* Include EXCEPT TABLE clause if there are except_tables. */

~~~

3b.
Although it works OK, I felt the following looked strange:
+ if (!first_tbl)
+ appendPQExpBufferStr(query, ")");

IMO it would be better implemented as a counter:

Replace
bool first_tbl = true;
with
int n_excluded = 0;

Then,
+ if (first_tbl)
+ {
+ appendPQExpBufferStr(query, " EXCEPT TABLE (");
+ first_tbl = false;
+ }
becomes
+ if (++n_excluded == 1)
+ appendPQExpBufferStr(query, " EXCEPT TABLE (");

And,
+ if (!first_tbl)
+ appendPQExpBufferStr(query, ")");
becomes
+ if (n_excluded > 0)
+ appendPQExpBufferStr(query, ")");

======
src/bin/psql/describe.c

describeOneTableDetails:

4.
+ /* Print publication the relation is excluded explicitly */
+ if (pset.sversion >= 190000)

The comment doesn't seem right:

SUGGESTION
Print publications that the table is explicitly excluded from

======
src/test/regress/sql/publication.sql

5.
Missing tests.

There are no test cases to show that \d is working for printing the
"Except Publications:".

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