On Wed, Sep 29, 2021 at 5:48 PM [email protected]
<[email protected]> wrote:
>
> On Wed, Sep 29, 2021 5:14 PM Amit Kapila <[email protected]> wrote:
> > On Wed, Sep 29, 2021 at 11:59 AM [email protected]
> > <[email protected]> wrote:
> > >
> > > On Wed, Sep 29, 2021 12:34 PM Amit Kapila <[email protected]>
> > > > On Wed, Sep 29, 2021 at 9:07 AM [email protected]
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Tues, Sep 28, 2021 10:46 PM vignesh C <[email protected]>
> > 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 same schema'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.
