On Thu, Sep 23, 2021 at 12:22 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > From Thurs, Sep 23, 2021 12:09 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Sep 22, 2021 at 5:03 PM Hou Zhijie <houzj.f...@fujitsu.com> wrote: > > > > > > Personally, I think we'd better move the code about changing publication's > > > tablelist into AlterPublicationTables and the code about changing > > publication's > > > schemalist into AlterPublicationSchemas. It's similar to what the > > v29-patchset > > > did, the difference is the SET action, I suggest we drop all the tables in > > > function AlterPublicationTables when user only set schemas and drop all > > > the > > > schema in AlterPublicationSchemas when user only set tables. In this > > approach, > > > we can keep schema and relation code separate and don't need to worry > > > about the locking order. > > > > > > Attach a top-up patch which refactor the code like above. > > > > > > > Good suggestion. I think it would still be better if we can move the > > checks related to superuser and puballtables into a separate function > > that gets called before taking a lock on publication. > > I agreed. > > I noticed v30-0001 has been committed with some minor changes, and the > V30-0002 > patchset need to be rebased accordingly. Attach a rebased version patch set to > make cfbot happy. Also Attach the two top-up patches which refactor the code > as > suggested. (top-up patch 1 is to keep schema and table code separate, top-up > patch 2 is to move some cheap check into a function and invoke it before > locking.)
Thanks for the patches, the changes simplifies alterpublications code and handles the drop object in a better way. I have merged it to 0001 patch at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm1R-xbQvz4LU5OXu3KKwbWOz3uDcT_YjRU6V0R5FZDYDg%40mail.gmail.com Regards, Vignesh
