From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: stepya@ukr.net, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #15724: Can't create foreign table as partition
Date: Tue, 25 Jun 2019 08:56:57 -0400
Message-ID: <20190625125657.GA27674@alvherre.pgsql> (raw)
In-Reply-To: <CA+HiwqEap3OigtkeT8thhovf7OP7tQqzjKc4OGW_VFB9E+2mKQ@mail.gmail.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: alvherre@2ndquadrant.com, amitlangote09@gmail.com, pavan.deolasee@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15724: Can't create foreign table as partition
In-Reply-To: <20190625125657.GA27674@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox