Hi Vignesh.
Some review comments for patch v65-0001.
======
doc/src/sgml/ref/alter_publication.sgml
1.
It seems that SET ALL TABLES is not supported if the publication
already has FOR TABLE.
e.g
alter publication pub1 set all tables;
ERROR: publication "pub1" does not support ALL TABLES operations
DETAIL: This operation requires the publication to be defined as FOR
ALL TABLES/SEQUENCES or to be empty.
e.g.
alter publication pub2 set table t1;
ERROR: publication "pub2" is defined as FOR ALL TABLES
DETAIL: Tables or sequences cannot be added to or dropped from FOR
ALL TABLES publications.
I am not going to debate what rules are right or wrong. (Some rules do
seem a bit ad-hoc to me, but maybe they are just erring on the safety
side). But my point is that the documentation does not seem to say
anything much about what the rules are
e.g. it says "Adding/Setting any schema when the publication also
publishes a table with a column list, and vice versa is not
supported.", but OTOH it says nothing about what is
supported/unsupported for SET ALL TABLES.
======
src/backend/catalog/pg_publication.c
2.
+/*
+ * Returns true if the publication has explicitly included relation (i.e.,
+ * not marked as EXCEPT).
+ */
+bool
+is_table_publication(Oid pubid)
To me, an "explicitly included relation" is like when you say "FOR
TABLE t1", where the "t1" is explicitly named.
So it's not very clear whether you consider "FOR ALL TABLES" or a
"FOR TABLES IN SCHEMA" publication explicitly includes tables or not?
The function comment needs to be clearer.
~~~
publication_add_relation:
3.
+ /*
+ * Determine whether EXCEPT tables require explicit relcache invalidation.
+ *
+ * For CREATE PUBLICATION with EXCEPT tables, invalidation is not needed,
+ * since it is handled when marking the publication as ALL TABLES.
+ *
+ * For ALTER PUBLICATION, invalidation is needed only when adding an EXCEPT
+ * table to a publication already marked as ALL TABLES. For publications
+ * that were originally empty or defined as ALL SEQUENCES and are being
+ * converted to ALL TABLES, invalidation is skipped here, as it is handled
+ * when marking the publication as ALL TABLES.
+ */
+ inval_except_table = (stmt != NULL) &&
+ (stmt->for_all_tables == pub->alltables);
3a.
Why is this code done at the top of the function? Can you move it to
be adjacent to where it is getting used?
~
3b.
I think 'alter_stmt' or 'alter_pub_stmt' might be a more informative
name here, instead of the generic 'stmt'
======
src/backend/commands/publicationcmds.c
4.
- TransformPubWhereClauses(rels, queryString, pubform->pubviaroot);
+ if (isexcept)
+ oldrelids = GetExcludedPublicationTables(pubid,
+ PUBLICATION_PART_ROOT);
+ else
+ {
+ oldrelids = GetIncludedPublicationRelations(pubid,
+ PUBLICATION_PART_ROOT);
I felt there were some subtle things that the logic is using here:
e.g.
It seems that because this function is called...
And because it is SET ....
And because it is SET ALL TABLES ...
Then, the tables can only be those in the EXCEPT TABLE list
Maybe more comments about 'isexcept', and maybe an Assert(oldrelids !=
NIL); could help here (??)
~~~
AlterPublicationAllFlags:
5.
+ bool nulls[Natts_pg_publication];
+ bool replaces[Natts_pg_publication];
+ Datum values[Natts_pg_publication];
+ bool dirty = false;
+
+ if (!stmt->for_all_tables && !stmt->for_all_sequences)
+ return;
+
+ pubform = (Form_pg_publication) GETSTRUCT(tup);
+
+ memset(values, 0, sizeof(values));
+ memset(nulls, false, sizeof(nulls));
+ memset(replaces, false, sizeof(replaces));
AFAIK, these memsets are not needed if you just say "= {0}" where
those vars are declared.
~~~
AlterPublication:
6.
+ relations = list_concat(relations, exceptrelations);
AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext,
schemaidlist != NIL);
I did not quite understand this list_concat.
Is this somehow asserting that `relations` must be empty when there
were `exceptrelations` and vice versa, because other combinations are
not supported by ALTER -- e.g. is this just a trick to so you can pass
the same parameter to AlterPublicationTables? This seems related to
the 'isexecpt' AlterPublicationTables function, which was also quite
subtle.
Bottom line is, I'm unsure what the logic is here, but it appears
overly tricky to me. Would more comments help?
======
src/backend/parser/gram.y
7.
+ * ALL TABLES [ EXCEPT TABLE ( table_name [, ...] ) ]
The CREATE/ALTER docs synopsis says "[ ONLY ] table_name [ * ]" which
is different from this comment. So are ONLY and * handled properly or
not?
======
src/include/nodes/parsenodes.h
8.
+ bool for_all_tables; /* True if SET ALL TABLES is specified */
+ bool for_all_sequences; /* True if SET ALL SEQUENCES is specified */
Maybe these comments do not need to mention the keyword "SET".
That way, in future if/when ADD/DROP get implemented, then this code
won't need to churn again.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
