Re: BUG #15724: Can't create foreign table as partition

On Tue, Jun 25, 2019 at 9:57 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> On 2019-Jun-25, Amit Langote wrote:
> > @@ -15705,6 +15721,32 @@ AttachPartitionEnsureIndexes(Relation rel,
> > Relation attachrel)
> > i++;
> > }
> >
> > + /*
> > + * If we're attaching a foreign table, we must fail if any of the indexes
> > + * is a constraint index; otherwise, there's nothing to do here.
> > + */
> > + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
> > + {
> > + foreach(cell, idxes)
> > + {
> > + Oid idx = lfirst_oid(cell);
> >
> > Why not add the is-foreign-table check in the loop that already exists
> > just below the above added code? That's what the patch does for
> > DefineRelation() and if you do so, there's no need for the goto label
> > that's also added by the patch.
>
> Because if you do that, you might build a few indexes on regular
> partitions before coming across a foreign one, which is very unfriendly.
> I'll add a comment to this effect.

Hmm, why would other partitions be involved?
AttachPartitionEnsureIndexes() only considers the partition being
attached, like DefineRelation only considers the partition being
created.

> > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > errmsg("cannot create index
> > on partitioned table \"%s\"",
> > stmt->relation->relname),
> > + errdetail("Table \"%s\"
> > contains weird partitions or something.",
> > + stmt->relation->relname)));
>
> > The "...weird partitions or something" message wouldn't be very
> > useful, but maybe you intended to rewrite it before committing?
>
> Hah, yeah, I did :-)
>
> > I suppose we could turn that particular ereport into elog(ERROR, ...),
> > because finding children of a partitioned that are neither of
> > RELATION, PARTITIONED_TABLE, FOREIGN_TABLE should be an internal
> > error.
>
> Yeah, an elog() sounds a good idea. I suppose "unexpected relkind
> \"%c\" on partition \"%s\"" should be good.

Yeah, that'll suffice.

> BTW I'm now thinking that those ERRCODE_INVALID_OBJECT_DEFINITION codes
> are not really the correct ones; I mean, it would be the right one to
> use for the unexpected relkind condition, but for the other cases I
> think we should use ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE instead.

Agreed.

Thanks,
Amit