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

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