Hi Vignesh. Unfortunately, IMO there are some fundamental problems here due to there being no accounting for publications with FOR ALL SEQUENCES. So I am posting my review comments for just the docs part so you can see it from my PoV. Maybe this was already discussed earlier in the thread but I saw no mentions of it. AFAICT the discussion/posts were mostly focussed on the setting/resetting of EXCEPT TABLE but seem to be overlooking the bigger picture.
Below review comments are for v63-0001 docs only. ====== Commit Message 1. The first form replaces the current EXCEPT TABLE list with the specified tables. The second form clears the existing except table list. Like the creation syntax, only root partitioned tables can be specified in the exclusion list. ~ IMO the second form is a long-time missing command from Postgres. For example, it is possible to create an "empty" publication. But without this "ALTER PUBLICATION name SET ALL TABLES" there was no way to convert that to be a "FOR ALL TABLES" publication. So really this feature was independently needed and should be done anyway irrespective of any side-effect it has for the EXCEPT TABLE list. Ideally the SET/ADD/DROP ALL TABLES can split out and done ahead of the EXCEPT TABLE stuff. Similarly, SET/ADD/DROP should be implemented also for FOR ALL SEQUENCES otherwise there is no way to manipulate those either. ====== doc/src/sgml/ref/alter_publication.sgml 2. Implementing the "SET ALL TABLES" is only a start. You also need to have the other case: * ADD ALL TABLES * DROP ALL TABLES Note you might start out with something like "CREATE PUBLICATION pub FOR ALL TABLES, FOR ALL SEQUENCES" so you need to be able to be able to modify/remove the published TABLES part without overwriting published SEQUENCES part!! i.e The "SET" means set the publication; it doesn't mean add to the publication. So SET ALL TABLES obliterates any FOR ALL SEQUENCES. ~~~ 3. +<phrase>where <replaceable class="parameter">publication_except_tables</replaceable> is:</phrase> + + [ EXCEPT TABLE ( [ ONLY ] <replaceable class="parameter">table_name</replaceable> [, ... ] ) ] + Hmm. Ideally, this should be part of 'publication_object' and publication_drop_object' because the ADD/DROP are also needed for the reasons given in my above review commentS. ~~~ 4. <para> - The first three variants change which tables/schemas are part of the - publication. The <literal>SET</literal> clause will replace the list of - tables/schemas in the publication with the specified list; the existing - tables/schemas that were present in the publication will be removed. The + The first four variants modify which tables/schemas are included in the + publication, or which tables are excluded from it. The + <literal>SET ALL TABLES</literal> clause is used to update the + <literal>EXCEPT TABLE</literal> list of a <literal>FOR ALL TABLES</literal> + publication. If <literal>EXCEPT TABLE</literal> is specified with a list of + tables, the existing except table list is replaced with the specified tables. + If <literal>EXCEPT TABLE</literal> is omitted, the existing except table + list is cleared. The <literal>SET</literal> clause, when used with a + publication defined with <literal>FOR TABLE</literal> or + <literal>FOR TABLES IN SCHEMA</literal>, replaces the list of tables/schemas + in the publication with the specified list; the existing tables or schemas + that were present in the publication will be removed. The I find this all a bit dubious because nothing seems to be accounting for the possibility of "FOR ALL SEQUENCES" also in the publication... e.g this entire ALTER command should also have SET ALL SEQUENCES ADD ALL SEQUENCES DROP ALL SEQUENCE and SET ALL TABLES ADD ALL TABLES DROP ALL TABLES IMO, a user will need to take care when using ALTER PUBLICATION ... SET ALL TABLES that it does not destroy the publication of sequences (and vice versa) -- Start with an "empty" publication and make it a "FOR ALL TABLES" publication... CREATE PUBLICATION pub; ALTER PUBLICATION pub SET ALL TABLES; -- result is equivalent to "CREATE PUBLICATION ... FOR ALL TABLES" -- give some table exceptions to it ALTER PUBLICATION pub SET ALL TABLES EXCEPT TABLE(t1,t2); -- result is equivalent to "CREATE PUBLICATION ... FOR ALL TABLES EXCEPT TABLE(t1,t2)" -- add sequences to this ALTER PUBLICATION pub ADD ALL SEQUENCES -- result is equivalent to "CREATE PUBLICATION ... FOR ALL TABLES EXCEPT TABLE(t1,t2), FOR ALL SEQUENCES" -- remove the table exception -- here you cannot simply use SET ALL TABLES because you will lose the ALL SEQUENCES part of the publication!! -- So, instead you need to do like below ALTER PUBLICATION pub DROP ALL TABLES; ALTER PUBLICATION pub ADD ALL TABLES; -- result is equivalent to "CREATE PUBLICATION ... FOR ALL TABLES, FOR ALL SEQUENCES" ~~~ 5. +</programlisting></para> + + <para> + Replace the publication's EXCEPT table list: /EXCEPT table list/EXCEPT TABLE list/ ~~~ 6. + <para> + Change the publication to include all tables by removing any existing + EXCEPT table list: The clearing of the EXCEPT TABLES is more like a side-effect, so I think this can be worded differently. SUGGESTION Reset the publication to be a FOR ALL TABLES publication with no excluded tables. ====== Kind Regards, Peter Smith. Fujitsu Australia
