On Mon, 17 Nov 2025 at 11:35, Peter Smith <[email protected]> wrote: > > On Tue, Nov 11, 2025 at 9:22 PM Shlok Kyal <[email protected]> wrote: > > > > On Fri, 7 Nov 2025 at 09:34, Peter Smith <[email protected]> wrote: > > > > > > Hi Shlok. > > > > > > This is a general comment about the content of these patches. > > > > > > IIUC, the v25* patches currently are currently arranged like this: > > > > > > 0001 > > > - New command ALTER PUBLICATION pubname RESET; > > > 0002 > > > - Add new command: ALTER PUBLICATION pub_name ADD ALL TABLES; > > > - Enhance existing CREATE and the new ALTER syntax for EXCEPT tables > > > 0003 > > > - Enhance existing CREATE and ALTER syntax for EXCEPT col_list > > > > > > ~~~ > > > > > > IMO it is a bug that the ALTER PUBLICATION pub_name ADD/SET ALL TABLES > > > command does not already exist as a supported command. And, that is > > > independent of anything else you are implementing here like RESET or > > > EXCEPT. > > > > > > Therefore, I think that one should be 1st in your patchset; The EXCEPT > > > stuff then just becomes enhancements to existing syntax, which would > > > give a cleaner separation of logic. > > > > > > So, I am suggesting there should be 4 patches instead of 3. e.g. > > > > > > SUGGESTION > > > 0001 - New command: ALTER PUBLICATION pub_name ADD/SET ALL TABLES; > > > 0002 - New command: ALTER PUBLICATION pubname RESET; > > > 0003 - Enhance existing CREATE/ALTER syntax for EXCEPT tables > > > 0004 - Enhance existing CREATE/ALTER syntax for EXCEPT col_list > > > > > I read the previous conversation in the thread. And got an > > understanding that RESET was introduced so that we can have a way to > > remove 'EXCEPT TABLE' from a publication and after RESET we can use > > 'ADD ALL TABLES [EXCEPT]' to alter the list of EXCEPT TABLE. So I > > prefer to keep 'ALTER PUBLICATION .. RESET' as the first patch. > > I think since 'ADD ALL TABLES' serves our current purpose. We can add > > the syntax 'SET ALL TABLES' once 'ADD ALL TABLES' is in committed or > > in committable shape. > > > > Sure, you can defer the ALTER PUBLICATION ... SET ALL TABLES. > > However, I still think that 'ALTER PUBLICATION ... ADD ALL TABLES' is > a self-contained new command that deserves to have its own *separate* > patch and tests and docs, etc. > > IMO, patch 0002 is doing too much at once. It would be tidier (and > smaller and easier to review, etc) if you split 0002 to implement the > new 'ALTER PUBLICATION ... ADD ALL TABLES' separately, before > expanding on that to implement the EXCEPT part: 'ALTER PUBLICATION ... > ADD ALL TABLES [EXCEPT ...]'.
I agree with you. I have split the 0002 patch. Now we have following patches 0001 - Add RESET clause to Alter Publication 0002 - Support ADD ALL TABLES in ALTER PUBLICATION 0003 - Skip publishing the tables specified in EXCEPT TABLE 0004 - Skip publishing the columns specified in FOR TABLE EXCEPT I have attached the updated patch in [1]. [1]: https://www.postgresql.org/message-id/CANhcyEXCKPCAdoqBLAhxt64Nwf%2B7T52dd8daE3qvhBNTvro13Q%40mail.gmail.com Thanks, Shlok Kyal
