On Wed, Sep 22, 2021 1:29 PMAmit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Sep 22, 2021 at 8:02 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wednesday, September 22, 2021 2:02 AM vignesh C
> <vignesh21@gmail.com> wrote:
> > > Attached v30 patch has the fixes for the same.
> >
> > Thanks for updating the patches.
> >
> > I have one comment.
> > @@ -474,7 +707,75 @@ AlterPublication(ParseState *pstate,
> > AlterPublicationStmt *stmt) ...
> > + if (list_length(relations))
> > + {
> > ...
> > + /* remove the existing schemas from the publication
> */
> > + PublicationDropSchemas(pubform->oid,
> > + delschemas, false);
> > ...
> > + }
> >
> > After more thoughts on it, I still don't think drop all the schemas
> > under " if (list_length(relations))" is a good idea. I think 1) we'd
> > better keep schema and relation code separate.
> >
>
> 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 ?
Best regards,
Hou zj