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

Hi Alvaro,

On Sat, Jun 22, 2019 at 12:49 AM Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> On 2019-Jun-21, Pavan Deolasee wrote:
> > On Thu, Jun 20, 2019 at 6:07 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> > wrote:
> >
> > > With this patch, an index creation will no longer fail in the presence
> > > of a partition that is a foreign table, as long as the index is not a
> > > constraint index (not unique, not primary key). Conversely,
> > > creating/attaching a partition that is a foreign table does not fail if
> > > the partitioned table only has non-constraint indexes.
> >
> > Like others suggested above, I also think that we should make this is a
> > no-op on the foreign tables i.e. not fail even when there exists a UNIQUE
> > or PRIMARY KEY on the parent table. We simply assume that the appropriate
> > constraints will be defined on the foreign side and violations will be
> > caught. This is same as CHECK constraints on the foreign partitions, that
> > we assume the foreign server will enforce.
>
> I don't oppose making such a change in pg13, but it seems too much of a
> behavior change to be making even in pg12, let alone in the stable
> branches.

I agree that index constraints on foreign tables would be a new
feature, because to support them we'd first need to support defining
(dummy) indexes on foreign tables.

> Therefore, my proposal --in response to this bug report-- is to
> backpatch the previously proposed patch, which allows indexes to be
> created [on partitioned tables containing foreign tables as partitions],
> as long as they are not UNIQUE/PKs.

I've looked at the patch you posted on Jun 20 and here are some comments.

@@ -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.

@@ -1347,11 +1347,20 @@ ProcessUtilitySlow(ParseState *pstate,

if (relkind != RELKIND_RELATION &&
relkind != RELKIND_MATVIEW &&
- relkind != RELKIND_PARTITIONED_TABLE)
+ relkind != RELKIND_PARTITIONED_TABLE &&
+ relkind != RELKIND_FOREIGN_TABLE)
ereport(ERROR,

(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)));
+ if (relkind == RELKIND_FOREIGN_TABLE &&
+ (stmt->unique || stmt->primary))
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot create unique
index on partitioned table \"%s\"",
+ stmt->relation->relname),

The "...weird partitions or something" message wouldn't be very
useful, but maybe you intended to rewrite it before committing? 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.

Also, +1 to back-patching, fwiw. Thanks for working on the patch.

Regards,
Amit