On Mon, 16 Mar 2026 at 14:08, Peter Smith <[email protected]> wrote: > > 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.
Thanks for the comments, the agreed comments have been addressed in the v64 version patch attached. Regards, Vignesh
v64-0001-Add-support-for-EXCEPT-TABLE-in-ALTER-PUBLICATIO.patch
Description: Binary data
