Date: Tue, 25 Jun 2019 08:56:57 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Pavan Deolasee <pavan.deolasee@gmail.com>, stepya@ukr.net,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #15724: Can't create foreign table as partition
Message-ID: <20190625125657.GA27674@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqEap3OigtkeT8thhovf7OP7tQqzjKc4OGW_VFB9E+2mKQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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