RE: Added schema level support for publication.

On Wed, Sep 29, 2021 5:14 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Sep 29, 2021 at 11:59 AM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Wed, Sep 29, 2021 12:34 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > > On Wed, Sep 29, 2021 at 9:07 AM houzj(dot)fnst(at)fujitsu(dot)com
> > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > >
> > > > On Tues, Sep 28, 2021 10:46 PM vignesh C <vignesh21(at)gmail(dot)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.

Besides, in the current patch, we have allowed similar case in CREATE/ALTER
PUBLICATION cases. In this SQL: "create publication pub for ALL TABLES IN
schema sch2, Table sch1.tbl1;", one of the partitions ' sch2.tbl1_part1' is
from schema 'sch2' which is published. It might be better to make the behavior
consistent.

> 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 same schema's table \"%s\" in the publication \"%s\" which is not supported.",
stmt->newschema,
RelationGetRelationName(rel),
get_publication_name(lfirst_oid(cell), false)));
}

Best regards,
Hou zj