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

On 2019-Mar-29, PG Bug reporting form wrote:

CREATE FOREIGN TABLE staging.msg_json PARTITION of msg_json
FOR VALUES FROM (20170101) TO (20180101)
SERVER postgres_big_data_arch
OPTIONS (schema_name 'fix_capture', table_name 'msg_json');

I got error !!!!

SQL Error [42809]: ERROR: cannot create index on foreign table "msg_json"
ERROR: cannot create index on foreign table "msg_json"
ERROR: cannot create index on foreign table "msg_json"

Ah, this is because we try to propagate the index to the partition,
which appears to be less than desirable. Maybe this patch fixes it? I
have not tested it, only verified that it compiles.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2019-Mar-29, PG Bug reporting form wrote:

SQL Error [42809]: ERROR: cannot create index on foreign table "msg_json"

Ah, this is because we try to propagate the index to the partition,
which appears to be less than desirable. Maybe this patch fixes it? I
have not tested it, only verified that it compiles.

Really, what *ought* to happen in such a case is that the FDW gets
told about the command and has the opportunity to try to do something
corresponding to making an index. But that smells like a new feature
rather than a bug fix.

I'm not sure that just forcing the case to be a no-op is wise.
What if the index is supposed to be UNIQUE?

regards, tom lane

On 2019-Mar-29, Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2019-Mar-29, PG Bug reporting form wrote:

SQL Error [42809]: ERROR: cannot create index on foreign table "msg_json"

Ah, this is because we try to propagate the index to the partition,
which appears to be less than desirable. Maybe this patch fixes it? I
have not tested it, only verified that it compiles.

Really, what *ought* to happen in such a case is that the FDW gets
told about the command and has the opportunity to try to do something
corresponding to making an index. But that smells like a new feature
rather than a bug fix.

ENOTIME to get it done for pg12 I suppose (I don't know a thing about
postgres_fdw or the FDW API layer).

I'm not sure that just forcing the case to be a no-op is wise.
What if the index is supposed to be UNIQUE?

Hmm, good point, that should be disallowed too.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I think we need at least to mention it in the dochttps://www.postgresql.org/docs/11/ddl-partitioning.html

 Partitions can also be foreign tables, although they have some limitations that normal tables do not; see CREATE FOREIGN TABLE for more information

For example  
  Partitions can also be foreign tables only in case main table is non indexed and doesn't contain PK, although they have some limitations that normal tables do not; see CREATE FOREIGN TABLE for more information

--- Original message ---
From: "Alvaro Herrera" <alvherre@2ndquadrant.com>
Date: 29 March 2019, 16:11:04

On 2019-Mar-29, Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2019-Mar-29, PG Bug reporting form wrote:

SQL Error [42809]: ERROR: cannot create index on foreign table "msg_json"

Ah, this is because we try to propagate the index to the partition,
which appears to be less than desirable. Maybe this patch fixes it? I
have not tested it, only verified that it compiles.

Really, what *ought* to happen in such a case is that the FDW gets
told about the command and has the opportunity to try to do something
corresponding to making an index. But that smells like a new feature
rather than a bug fix.

ENOTIME to get it done for pg12 I suppose (I don't know a thing about
postgres_fdw or the FDW API layer).

I'm not sure that just forcing the case to be a no-op is wise.
What if the index is supposed to be UNIQUE?

Hmm, good point, that should be disallowed too.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I'm not sure that just forcing the case to be a no-op is wise.
What if the index is supposed to be UNIQUE?Anyway it would be better to have no-op in 11.3 than having it not working. We just can put comment in the doc like. Foreign Server should take care about indexing foreign table as partition.

--- Original message ---
From: "Tom Lane" <tgl@sss.pgh.pa.us>
Date: 29 March 2019, 16:01:41

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2019-Mar-29, PG Bug reporting form wrote:

SQL Error [42809]: ERROR: cannot create index on foreign table "msg_json"

Ah, this is because we try to propagate the index to the partition,
which appears to be less than desirable. Maybe this patch fixes it? I
have not tested it, only verified that it compiles.

Really, what *ought* to happen in such a case is that the FDW gets
told about the command and has the opportunity to try to do something
corresponding to making an index. But that smells like a new feature
rather than a bug fix.

I'm not sure that just forcing the case to be a no-op is wise.
What if the index is supposed to be UNIQUE?

regards, tom lane

(2019/03/29 23:10), Alvaro Herrera wrote:

On 2019-Mar-29, Tom Lane wrote:

Alvaro Herrera<alvherre@2ndquadrant.com> writes:

On 2019-Mar-29, PG Bug reporting form wrote:

SQL Error [42809]: ERROR: cannot create index on foreign table "msg_json"

Really, what *ought* to happen in such a case is that the FDW gets
told about the command and has the opportunity to try to do something
corresponding to making an index.

Interesting!

corresponding to making an index. But that smells like a new feature
rather than a bug fix.

ENOTIME to get it done for pg12 I suppose (I don't know a thing about
postgres_fdw or the FDW API layer).

Yeah, I think so.

I'm not sure that just forcing the case to be a no-op is wise.
What if the index is supposed to be UNIQUE?

Hmm, good point, that should be disallowed too.

Agreed.

Best regards,
Etsuro Fujita

(2019/03/29 23:21), Stepan Yankevych wrote:

I think we need at least to mention it in the doc
https://www.postgresql.org/docs/11/ddl-partitioning.html

Partitions can also be foreign tables, although they have some

limitations that normal tables do not; see CREATE FOREIGN TABLE
<https://www.postgresql.org/docs/11/sql-createforeigntable.html&gt;for more
information
For example
Partitions can also be foreign tables*only in case main table is non
indexed and doesn't contain PK,*although they have some limitations that
normal tables do not; see CREATE FOREIGN TABLE
<https://www.postgresql.org/docs/11/sql-createforeigntable.html&gt;for more
information

+1 for adding such notes, but I'm not sure if it's a good idea to put it
there because I think that would make the introductory documentation a
bit verbose. How about adding it to the reference pages of CREATE
FOREIGN TABLE and ALTER TABLE like the attached?

Best regards,
Etsuro Fujita

On 2019/04/01 14:35, Etsuro Fujita wrote:

(2019/03/29 23:10), Alvaro Herrera wrote:

On 2019-Mar-29, Tom Lane wrote:

Alvaro Herrera<alvherre@2ndquadrant.com>  writes:

On 2019-Mar-29, PG Bug reporting form wrote:

SQL Error [42809]: ERROR: cannot create index on foreign table
"msg_json"

Really, what *ought* to happen in such a case is that the FDW gets
told about the command and has the opportunity to try to do something
corresponding to making an index.

Interesting!

+1

Maybe, CHECK constraints on foreign tables are similar -- it merely
asserts the condition that all of the rows in the remote table *must*
satisfy so that the local server can do something useful with it, such as
trying to exclude the table with constraint exclusion.

I guess the intended benefit with allowing indexes would be similar -- a
locally-defined index on a foreign table will help an FDW estimate the
cost of a query accessing the table more efficiently than, for example,
having to rely on some other relatively expensive alternative, such as
use_remote_estimate in postgres_fdw's case.

Thanks,
Amit

(2019/03/30 1:25), Stepan Yankevych wrote:

I'm not sure that just forcing the case to be a no-op is wise.
What if the index is supposed to be UNIQUE?

Anyway it would be better to have no-op in 11.3 than having it not working.

We just can put comment in the doc like.

Foreign Server should take care about indexing foreign table as partition.

That might be OK, but before doing that, we would need to support eg,
unique constraints on foreign tables, so this seems more like a feature
than a fix to me.

Best regards,
Etsuro Fujita

Fujita-san,

On 2019/04/01 15:01, Etsuro Fujita wrote:

(2019/03/29 23:21), Stepan Yankevych wrote:

I think we need at least to mention it in the doc
https://www.postgresql.org/docs/11/ddl-partitioning.html

Partitions can also be foreign tables, although they have some

limitations that normal tables do not; see CREATE FOREIGN TABLE
<https://www.postgresql.org/docs/11/sql-createforeigntable.html&gt;for more
information
For example
Partitions can also be foreign tables*only in case main table is non
indexed and doesn't contain PK,*although they have some limitations that
normal tables do not; see CREATE FOREIGN TABLE
<https://www.postgresql.org/docs/11/sql-createforeigntable.html&gt;for more
information

+1 for adding such notes, but I'm not sure if it's a good idea to put it
there because I think that would make the introductory documentation a bit
verbose.  How about adding it to the reference pages of CREATE FOREIGN
TABLE and ALTER TABLE like the attached?

Thanks for creating the patch. Looks good to me.

Thanks,
Amit

(2019/04/01 15:23), Amit Langote wrote:

On 2019/04/01 14:35, Etsuro Fujita wrote:

(2019/03/29 23:10), Alvaro Herrera wrote:

On 2019-Mar-29, Tom Lane wrote:

Alvaro Herrera<alvherre@2ndquadrant.com> writes:

On 2019-Mar-29, PG Bug reporting form wrote:

SQL Error [42809]: ERROR: cannot create index on foreign table
"msg_json"

Really, what *ought* to happen in such a case is that the FDW gets
told about the command and has the opportunity to try to do something
corresponding to making an index.

Interesting!

+1

Maybe, CHECK constraints on foreign tables are similar -- it merely
asserts the condition that all of the rows in the remote table *must*
satisfy so that the local server can do something useful with it, such as
trying to exclude the table with constraint exclusion.

I guess the intended benefit with allowing indexes would be similar -- a
locally-defined index on a foreign table will help an FDW estimate the
cost of a query accessing the table more efficiently than, for example,
having to rely on some other relatively expensive alternative, such as
use_remote_estimate in postgres_fdw's case.

Yeah, I think so. Another benefit I can think of would be: that would
probably allow us to support ON CONFLICT DO UPDATE for foreign tables.
I think the intended benefit would be large!

Best regards,
Etsuro Fujita

(2019/04/01 15:45), Amit Langote wrote:

On 2019/04/01 15:01, Etsuro Fujita wrote:

+1 for adding such notes, but I'm not sure if it's a good idea to put it
there because I think that would make the introductory documentation a bit
verbose. How about adding it to the reference pages of CREATE FOREIGN
TABLE and ALTER TABLE like the attached?

Thanks for creating the patch. Looks good to me.

Pushed after fixing a copy-pasto: s/sql-createtable/sql-createforeigntable/

Thanks for reviewing, Amit-san!

Best regards,
Etsuro Fujita

On 2019-Mar-29, Alvaro Herrera wrote:

On 2019-Mar-29, PG Bug reporting form wrote:

CREATE FOREIGN TABLE staging.msg_json PARTITION of msg_json
FOR VALUES FROM (20170101) TO (20180101)
SERVER postgres_big_data_arch
OPTIONS (schema_name 'fix_capture', table_name 'msg_json');

I got error !!!!

SQL Error [42809]: ERROR: cannot create index on foreign table "msg_json"
ERROR: cannot create index on foreign table "msg_json"
ERROR: cannot create index on foreign table "msg_json"

Ah, this is because we try to propagate the index to the partition,
which appears to be less than desirable. Maybe this patch fixes it? I
have not tested it, only verified that it compiles.

The previously proposed patch doesn't work; there are a few other places
that needed patching. Here's a proposed patch. I'm not sure whether
this is a bug fix or a new feature.

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.

With the current code, these commands would all fail, and the resulting
behavior is pretty unhelpful, as reported in this thread and elsewhere.

Any opinions on backpatching this? I'm considering pg11, but also
considering not to backpatch at all.

(There *is* a much smaller bugfix here, which is that we don't raise an
error when unique/pk indexes exist and a foreign table is attached as
partition.)

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Jun 20, 2019 at 6:07 AM Alvaro Herrera <alvherre@2ndquadrant.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.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2019-Jun-21, Pavan Deolasee wrote:

On Thu, Jun 20, 2019 at 6:07 AM Alvaro Herrera <alvherre@2ndquadrant.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.

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.

If others want to explore the implications of UNIQUE/PK indexes on
partitioned tables with foreign partitions, I won't stand in their way,
though it doesn't seem material for this thread.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Alvaro,

On Sat, Jun 22, 2019 at 12:49 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

On 2019-Jun-21, Pavan Deolasee wrote:

On Thu, Jun 20, 2019 at 6:07 AM Alvaro Herrera <alvherre@2ndquadrant.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

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

On Tue, Jun 25, 2019 at 9:57 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

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.

Hmm, why would other partitions be involved?
AttachPartitionEnsureIndexes() only considers the partition being
attached, like DefineRelation only considers the partition being
created.

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

Yeah, that'll suffice.

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.

Agreed.

Thanks,
Amit

On 2019-Jun-26, Amit Langote wrote:

On Tue, Jun 25, 2019 at 9:57 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

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.

Hmm, why would other partitions be involved?
AttachPartitionEnsureIndexes() only considers the partition being
attached, like DefineRelation only considers the partition being
created.

Sorry, I meant other *indexes*. You might build a few regular
(non-unique) indexes before coming across a unique index, at which point
you throw the error.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services