On Thu, Sep 23, 2021 at 12:32 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thur, Sep 23, 2021 11:06 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
> > On Wed, Sep 22, 2021 at 9:33 PM houzj.fnst@fujitsu.com <houzj.fnst@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