Re: Added schema level support for publication.

On Wed, Sep 29, 2021 at 5:48 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wed, Sep 29, 2021 5:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Wed, Sep 29, 2021 at 11:59 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Wed, Sep 29, 2021 12:34 PM Amit Kapila <amit.kapila16@gmail.com>
> > > > On Wed, Sep 29, 2021 at 9:07 AM houzj.fnst@fujitsu.com
> > > > <houzj.fnst@fujitsu.com> wrote:
> > > > >
> > > > > On Tues, Sep 28, 2021 10:46 PM vignesh C <vignesh21@gmail.com>
> > wrote:
> > > > > > Attached v34 patch has the changes for the same.
> > > > > How about we check this case like the following ?
> > > > >
> > > > > List       *schemaPubids = GetSchemaPublications(nspOid);
> > > > > List       *relPubids = GetRelationPublications(RelationGetRelid(rel));
> > > > > if (list_intersection(schemaPubids, relPubids))
> > > > >         ereport(ERROR, ...
> > > > >
> > > >
> > > > Won't this will allow changing one of the partitions for which only
> > > > partitioned table is part of the target schema?
> > >
> > > I think it still disallow changing partition's schema to the published one.
> > > I tested with the following SQLs.
> > > -----
> > > create schema sch1;
> > > create schema sch2;
> > > create schema sch3;
> > >
> > > create table sch1.tbl1 (a int) partition by range ( a ); create table
> > > sch2.tbl1_part1 partition of sch1.tbl1 for values from (1) to (101);
> > > create table sch3.tbl1_part2 partition of sch1.tbl1 for values from
> > > (101) to (200); create publication pub for ALL TABLES IN schema sch1,
> > > TABLE sch2.tbl1_part1; alter table sch2.tbl1_part1 set schema sch1;
> > > ---* It will report an error here *
> > > -----
> > >
> >
> > Use all steps before "create publication" and then try below. These will give an
> > error with the patch proposed but if I change it to what you are proposing then
> > it won't give an error.
> > create publication pub for ALL TABLES IN schema sch2, Table sch1.tbl1; alter
> > table sch3.tbl1_part2 set schema sch2;
> >
> > But now again thinking about it, I am not sure if we really want to give error in
> > this case. What do you think?
>
> Personally, I think we can allow the above case.
>
> Because if user specify the partitioned table in the publication like above,
> they cannot drop the partition separately. And the partitioned table is the
> actual one in pg_publication_rel. So, I think allowing this case seems won't
> make people feel confused.
>

Yeah, I also thought on similar lines. So, let's allow this case.

>
> > Also, if we use list_intersection trick, then how will
> > we tell the publication due to which this problem has occurred, or do you think
> > we should leave that as an exercise for the user?
>
> I thought list_intersection will return a puboids list in which the puboid exists in both input list.
> We can choose the first puboid and output it in the error message which seems the same as
> the current patch.
>
> But I noticed list_intersection doesn't support T_OidList, so we might need to search the puboid
> Manually. Like:
>
>         foreach(cell, relPubids)
>         {
>                 if (list_member_oid(schemaPubids, lfirst_oid(cell)))
>                                 ereport(ERROR,
>                                                 errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                                                 errmsg("cannot move table \"%s\" to schema \"%s\"",
>                                                            RelationGetRelationName(rel), stmt->newschema),
>                                                 errdetail("Altering table will result in having schema \"%s\" and
sameschema's table \"%s\" in the publication \"%s\" which is not supported.",
 
>                                                                   stmt->newschema,
>                                                                   RelationGetRelationName(rel),
>                                                                   get_publication_name(lfirst_oid(cell), false)));
>         }
>

Looks good to me.


-- 
With Regards,
Amit Kapila.