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.
Best regards,
Hou zj
