On Thu, Sep 23, 2021 at 12:22 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> From Thurs, Sep 23, 2021 12:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Wed, Sep 22, 2021 at 5:03 PM Hou Zhijie <houzj.fnst@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
