On Thu, Sep 23, 2021 at 12:32 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > On Thur, Sep 23, 2021 11:06 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Wed, Sep 22, 2021 at 9:33 PM houzj.f...@fujitsu.com > > <houzj.f...@fujitsu.com> wrote: > > > > > > > > > > > How do you suggest changing it? > > > > > > 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. > > > Thoughts ? > > > > > > > Sounds like a good idea. > > Is it possible to incorporate the existing > > CheckPublicationAlterTables() and CheckPublicationAlterSchemas() functions > > into your suggested update? > > I think it might tidy up the error-checking a bit. > > I agreed we can put the check about ALL TABLE and superuser into a function > like what the v30-patchset did. But I have some hesitations about the code > related to CheckObjSchemaNotAlreadyInPublication(). Currently, we need to open > and lock the table before invoking the CheckObjxxx function, ISTM we'd better > open the table in function AlterPublicationTables. Maybe we can wait for the > author's(Vignesh) opinion.
I felt keeping the code related to CheckObjSchemaNotAlreadyInPublication as it is in AlterPublicationTables and AlterPublicationSchemas is better. Regards, Vignesh
