Hi Amit, thanks for reviewing.
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.
> (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.
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.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services