Re: Skipping schema changes in publication

Hi Shlok.

Some review comments for patch v33-0001 (code part)

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

GetPublicationRelationsInternal:

1.
Static function names should be snake_case.

~~~

GetPublicationIncludedRelations:

2.
+/*
+ * Return the list of relation OIDs for a publication.
+ *
+ * For a FOR TABLE publication, this returns the list of relations explicitly
+ * included in the publication.
+ *
+ * Publications declared with FOR ALL TABLES or FOR ALL SEQUENCES should use
+ * GetAllPublicationRelations() to obtain the complete set of tables covered by
+ * the publication.
+ */
+List *
+GetPublicationIncludedRelations(Oid pubid, PublicationPartOpt pub_partopt)
+{
+ Assert(!GetPublication(pubid)->alltables);
+
+ return GetPublicationRelationsInternal(pubid, pub_partopt, false);
+}

Why isn't the Assert also saying something about puballsequences, as
mentioned in the function comment?

~~~

GetAllPublicationRelations:

3.
+ * root partitioned tables. The list also excludes tables that are
+ * explicitly excluded via the EXCEPT TABLE clause of the publication
+ * identified by pubid. Neither of these rules applies to FOR ALL SEQUENCES
+ * publications.

3.
It seems wrong to say "FOR ALL SEQUENCES" ... that seems to assume the
"FOR ALL SEQUENCES" and "FOR ALL TABLES" cannot co-exist. Did you mean
"Neither of ... to published sequences"?

~

4.
-GetAllPublicationRelations(char relkind, bool pubviaroot)
+GetAllPublicationRelations(Oid pubid, char relkind, bool pubviaroot)

There are tricky rules about relation vs sequences and the
publish_via_partition_root parameter value. It would be better if you
encapsulate all this within this function. Specifically, it would be
simpler if you passed the 'Publication' arg instead of the pubid. Then
you can get the pubviaroot value from that (within the function)
instead of passing around "fake" values of false when you are looking
at RELKIND_SEQUENCE.
======
src/backend/commands/publicationcmds.c

ObjectsInAllPublicationToOids:

5.
+ foreach_ptr(PublicationAllObjSpec, puballobj, puballobjspec_list)
+ {
+ if (puballobj->pubobjtype != PUBLICATION_ALL_TABLES)
+ continue;
+
+ foreach_ptr(PublicationObjSpec, pubobj, puballobj->except_tables)
+ {
+ pubobj->pubtable->except = true;
+ *rels = lappend(*rels, pubobj->pubtable);
+ }
+ }

I think it's tidier to code this like below:

if (puballobj->pubobjtype == PUBLICATION_ALL_TABLES)
{
foreach_ptr...
}

~~~

pub_contains_invalid_column:

6.
bool
pub_contains_invalid_column(Oid pubid, Relation relation, List *ancestors,
bool pubviaroot, char pubgencols_type,
- bool *invalid_column_list,
- bool *invalid_gen_col)
+ bool *invalid_column_list, bool *invalid_gen_col)

Why does this change even exist at all in this patch?

~~~

CreatePublication:

7.
+ /*
+ * If the publication is for ALL TABLES and 'relations' is not empty, it
+ * indicates that some relations should be excluded from the publication.
+ * Add those excluded relations to the publication with 'prexcept' set to
+ * true. Otherwise, 'relations' contains the list of relations to be
+ * explicitly included in the publication.
+ */
+ if (relations != NIL)
+ {
+ List *rels;
+
+ rels = OpenTableList(relations);
+ TransformPubWhereClauses(rels, pstate->p_sourcetext,
+ publish_via_partition_root);
+
+ CheckPubRelationColumnList(stmt->pubname, rels,
+ schemaidlist != NIL,
+ publish_via_partition_root);
+
+ PublicationAddTables(puboid, rels, true, NULL);
+ CloseTableList(rels);
+ }
+

The comment and the code don't match. The comment is talking about
rules for FOR ALL TABLES, but puballtables is not part of any
condition here (??). Was all this supposed to be within the "if
(stmt->for_all_tables)" code block?

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

8.
- "SELECT tableoid, oid, prpubid, prrelid, "
+ "SELECT tableoid, oid, prpubid, prrelid,\n"
"pg_catalog.pg_get_expr(prqual, prrelid) AS prrelqual, "
"(CASE\n"
" WHEN pr.prattrs IS NOT NULL THEN\n"
@@ -4868,6 +4929,9 @@ getPublicationTables(Archive *fout, TableInfo
tblinfo[], int numTables)
" WHERE attrelid = pr.prrelid AND attnum = prattrs[s])\n"
" ELSE NULL END) prattrs "
"FROM pg_catalog.pg_publication_rel pr");
+ if (fout->remoteVersion >= 190000)
+ appendPQExpBufferStr(query, " WHERE prexcept = false");

8a
Isn't it better to qualify everything here with the alias 'pr'?

~

8b.
Also "WHERE NOT pr.prexcept;" might be more conssitent with other code
I saw in describe.c

======
src/bin/pg_dump/pg_dump.h

9.
PublishGencolsType pubgencols_type;
+ SimplePtrList excepttbls;
} PublicationInfo;

How about "tables instead of "tbls" (e.g. "excepttables" or
"except_tables") here? That would also be more consistent with the
other puballtables member.

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

10.
RESET client_min_messages;
\dRp+ testpub3
\dRp+ testpub4
+\dRp+ testpub5
+\dRp+ testpub6
+\dRp+ testpub7

I feel it would be better to keep each \dRp+ together with the test it
belongs to, rather than have a bunch of different tests which are then
followed by a bunch of different \dRp+. Note: this same comment
applies to other place of places -- not just here. Check everywhere
you do \dRp+

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