Thread: Re: Update does not move row across foreign partitions in v11
On Tue, 5 Mar 2019 at 03:01, Derek Hans <derek.hans@gmail.com> wrote: > Based on a reply to reporting this as a bug, moving rows out of foreign partitions is not yet implemented so this is behavingas expected. There's a mention of this limitation in the Notes section of the Update docs. (Moving this discussion to -Hackers) In [1], Derek reports that once a row is inserted into a foreign partition that an UPDATE does not correctly route it back out into the correct partition. I didn't really follow the foreign partition code when it went in, but do recall being involved in the documentation about the limitations of partitioned tables in table 5.10.2.3 in [2]. Unfortunately, table 5.10.2.3 does not seem to mention this limitation at all. As Derek mentions, there is a brief mention in [3] in the form of: "Currently, rows cannot be moved from a partition that is a foreign table to some other partition, but they can be moved into a foreign table if the foreign data wrapper supports it." I don't quite understand what a "foreign table to some other partition" is meant to mean. Partitions don't have foreign tables, they can only be one themselves. I've tried to put all this right again in the attached. However, I was a bit unsure of what "but they can be moved into a foreign table if the foreign data wrapper supports it." is referring to. Copying Robert and Etsuro as this was all added in 3d956d9562aa. Hopefully, they can confirm what is meant by this. [1] https://www.postgresql.org/message-id/CAGrP7a3Xc1Qy_B2WJcgAD8uQTS_NDcJn06O5mtS_Ne1nYhBsyw@mail.gmail.com [2] https://www.postgresql.org/docs/devel/ddl-partitioning.html#DDL-PARTITIONING-DECLARATIVE-LIMITATIONS [3] https://www.postgresql.org/docs/devel/sql-update.html -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Hi David, On 2019/03/06 11:06, David Rowley wrote: > On Tue, 5 Mar 2019 at 03:01, Derek Hans <derek.hans@gmail.com> wrote: >> Based on a reply to reporting this as a bug, moving rows out of foreign partitions is not yet implemented so this is behavingas expected. There's a mention of this limitation in the Notes section of the Update docs. > > (Moving this discussion to -Hackers) > > In [1], Derek reports that once a row is inserted into a foreign > partition that an UPDATE does not correctly route it back out into the > correct partition. > > I didn't really follow the foreign partition code when it went in, but > do recall being involved in the documentation about the limitations of > partitioned tables in table 5.10.2.3 in [2]. Unfortunately, table > 5.10.2.3 does not seem to mention this limitation at all. As Derek > mentions, there is a brief mention in [3] in the form of: > > "Currently, rows cannot be moved from a partition that is a foreign > table to some other partition, but they can be moved into a foreign > table if the foreign data wrapper supports it." > > I don't quite understand what a "foreign table to some other > partition" is meant to mean. Partitions don't have foreign tables, > they can only be one themselves. > > I've tried to put all this right again in the attached. However, I was > a bit unsure of what "but they can be moved into a foreign table if > the foreign data wrapper supports it." is referring to. Copying Robert > and Etsuro as this was all added in 3d956d9562aa. Hopefully, they can > confirm what is meant by this. Did you miss my reply on that thread? https://www.postgresql.org/message-id/CA%2BHiwqF3gma5HfCJb4_cOk0_%2BLEpVc57EHdBfz_EKt%2BNu0hNYg%40mail.gmail.com Thanks, Amit
On Wed, 6 Mar 2019 at 15:26, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > > I've tried to put all this right again in the attached. However, I was > > a bit unsure of what "but they can be moved into a foreign table if > > the foreign data wrapper supports it." is referring to. Copying Robert > > and Etsuro as this was all added in 3d956d9562aa. Hopefully, they can > > confirm what is meant by this. > > Did you miss my reply on that thread? > > https://www.postgresql.org/message-id/CA%2BHiwqF3gma5HfCJb4_cOk0_%2BLEpVc57EHdBfz_EKt%2BNu0hNYg%40mail.gmail.com Yes. I wasn't aware that there were two threads for this. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2019/03/06 11:29, David Rowley wrote: > On Wed, 6 Mar 2019 at 15:26, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> >>> I've tried to put all this right again in the attached. However, I was >>> a bit unsure of what "but they can be moved into a foreign table if >>> the foreign data wrapper supports it." is referring to. Copying Robert >>> and Etsuro as this was all added in 3d956d9562aa. Hopefully, they can >>> confirm what is meant by this. >> >> Did you miss my reply on that thread? >> >> https://www.postgresql.org/message-id/CA%2BHiwqF3gma5HfCJb4_cOk0_%2BLEpVc57EHdBfz_EKt%2BNu0hNYg%40mail.gmail.com > > Yes. I wasn't aware that there were two threads for this. Ah, indeed. In the documentation fix patch I'd posted, I also made changes to release-11.sgml to link to the limitations section. (I'm attaching it here for your reference.) Thanks, Amit
Attachment
(2019/03/06 11:06), David Rowley wrote: > On Tue, 5 Mar 2019 at 03:01, Derek Hans<derek.hans@gmail.com> wrote: >> Based on a reply to reporting this as a bug, moving rows out of foreign partitions is not yet implemented so this is behavingas expected. There's a mention of this limitation in the Notes section of the Update docs. > > (Moving this discussion to -Hackers) > > In [1], Derek reports that once a row is inserted into a foreign > partition that an UPDATE does not correctly route it back out into the > correct partition. > > I didn't really follow the foreign partition code when it went in, but > do recall being involved in the documentation about the limitations of > partitioned tables in table 5.10.2.3 in [2]. Unfortunately, table > 5.10.2.3 does not seem to mention this limitation at all. As Derek > mentions, there is a brief mention in [3] in the form of: > > "Currently, rows cannot be moved from a partition that is a foreign > table to some other partition, but they can be moved into a foreign > table if the foreign data wrapper supports it." > > I don't quite understand what a "foreign table to some other > partition" is meant to mean. Partitions don't have foreign tables, > they can only be one themselves. I think "foreign table" is describing "partition" in front of that; "a partition that is a foreign table". > I've tried to put all this right again in the attached. However, I was > a bit unsure of what "but they can be moved into a foreign table if > the foreign data wrapper supports it." is referring to. Copying Robert > and Etsuro as this was all added in 3d956d9562aa. Hopefully, they can > confirm what is meant by this. That means that rows can be moved from a local partition to a foreign partition if the FDW supports it. IMO, I think the existing mention in [3] is good, so I would vote for putting the same mention in table 5.10.2.3 in [2] as well. Best regards, Etsuro Fujita
On Wed, 6 Mar 2019 at 16:29, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
>
> (2019/03/06 11:06), David Rowley wrote:
> > I don't quite understand what a "foreign table to some other
> > partition" is meant to mean. Partitions don't have foreign tables,
> > they can only be one themselves.
>
> I think "foreign table" is describing "partition" in front of that; "a
> partition that is a foreign table".
I think I was reading this wrong:
- Currently, rows cannot be moved from a partition that is a
- foreign table to some other partition, but they can be moved into a foreign
- table if the foreign data wrapper supports it.
I parsed it as "cannot be moved from a partition, that is a foreign
table to some other partition"
and subsequently struggled with what "a foreign table to some other
partition" is.
but now looking at it, I think it's meant to mean:
"cannot be moved from a foreign table partition to another partition"
> > I've tried to put all this right again in the attached. However, I was
> > a bit unsure of what "but they can be moved into a foreign table if
> > the foreign data wrapper supports it." is referring to. Copying Robert
> > and Etsuro as this was all added in 3d956d9562aa. Hopefully, they can
> > confirm what is meant by this.
>
> That means that rows can be moved from a local partition to a foreign
> partition if the FDW supports it.
It seems a bit light on detail to me. If I was a user I'd want to know
what exactly the FDW needed to support this. Does it need a special
partition move function? Looking at ExecFindPartition(), this check
seems to be done in CheckValidResultRel() and is basically:
case RELKIND_FOREIGN_TABLE:
/* Okay only if the FDW supports it */
fdwroutine = resultRelInfo->ri_FdwRoutine;
switch (operation)
{
case CMD_INSERT:
if (fdwroutine->ExecForeignInsert == NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot insert into foreign table \"%s\"",
RelationGetRelationName(resultRel))));
Alternatively, we could just remove the mention about "if the FDW
supports it", since it's probably unlikely for an FDW not to support
INSERT.
> IMO, I think the existing mention in [3] is good, so I would vote for
> putting the same mention in table 5.10.2.3 in [2] as well.
I think the sentence is unclear, at least I struggled to parse it the
first time. Happy for Amit to choose some better words and include in
his patch. I think it should be done in the same commit.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
(2019/03/06 11:34), Amit Langote wrote: > Ah, indeed. In the documentation fix patch I'd posted, I also made > changes to release-11.sgml to link to the limitations section. (I'm > attaching it here for your reference.) I'm not sure it's a good idea to make changes to the release notes like that, because 1) that would make the release notes verbose, and 2) it might end up doing the same thing to items that have some limitations in the existing/future release notes (eg, FOR EACH ROW triggers on partitioned tables added to V11 has the limitation listed on the limitation section, so the same link would be needed.), for consistency. Best regards, Etsuro Fujita
Fujita-san, On 2019/03/06 13:04, Etsuro Fujita wrote: > (2019/03/06 11:34), Amit Langote wrote: >> Ah, indeed. In the documentation fix patch I'd posted, I also made >> changes to release-11.sgml to link to the limitations section. (I'm >> attaching it here for your reference.) > > I'm not sure it's a good idea to make changes to the release notes like > that, because 1) that would make the release notes verbose, and 2) it > might end up doing the same thing to items that have some limitations in > the existing/future release notes (eg, FOR EACH ROW triggers on > partitioned tables added to V11 has the limitation listed on the > limitation section, so the same link would be needed.), for consistency. OK, sure. It just seemed to me that the original complainer found it quite a bit surprising that such a limitation is not mentioned in the release notes, but maybe that's fine. It seems we don't normally list feature limitations in the release notes, which as you rightly say, would make them verbose. The main problem here is indeed that the limitation is not listed under the partitioning limitations in ddl.sgml, where it's easier to notice than in the UPDATE's page. I've updated my patch to remove the release-11.sgml changes. Thanks, Amit
Attachment
On 2019/03/06 12:47, David Rowley wrote:
> On Wed, 6 Mar 2019 at 16:29, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
>> That means that rows can be moved from a local partition to a foreign
>> partition if the FDW supports it.
>
> It seems a bit light on detail to me. If I was a user I'd want to know
> what exactly the FDW needed to support this. Does it need a special
> partition move function? Looking at ExecFindPartition(), this check
> seems to be done in CheckValidResultRel() and is basically:
>
> case RELKIND_FOREIGN_TABLE:
> /* Okay only if the FDW supports it */
> fdwroutine = resultRelInfo->ri_FdwRoutine;
> switch (operation)
> {
> case CMD_INSERT:
> if (fdwroutine->ExecForeignInsert == NULL)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot insert into foreign table \"%s\"",
> RelationGetRelationName(resultRel))));
>
> Alternatively, we could just remove the mention about "if the FDW
> supports it", since it's probably unlikely for an FDW not to support
> INSERT.
AFAIK, there's no special support in FDWs for "tuple moving" as such. The
"if the FDW supports it" refers to the FDW's ability to handle tuple
routing. Note that moving/re-routing involves calling
ExecPrepareTupleRouting followed by ExecInsert on the new tupls after the
old tuple is deleted. If an FDW doesn't support tuple routing, then a
tuple cannot be moved into it. That's what that text is talking about.
Maybe, we should reword it as "if the FDW supports tuple routing", so that
a reader doesn't go looking around for "tuple moving support" in FDWs.
Thanks,
Amit
On Wed, 6 Mar 2019 at 17:20, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2019/03/06 12:47, David Rowley wrote:
> > It seems a bit light on detail to me. If I was a user I'd want to know
> > what exactly the FDW needed to support this. Does it need a special
> > partition move function? Looking at ExecFindPartition(), this check
> > seems to be done in CheckValidResultRel() and is basically:
> >
> > case RELKIND_FOREIGN_TABLE:
> > /* Okay only if the FDW supports it */
> > fdwroutine = resultRelInfo->ri_FdwRoutine;
> > switch (operation)
> > {
> > case CMD_INSERT:
> > if (fdwroutine->ExecForeignInsert == NULL)
> > ereport(ERROR,
> > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > errmsg("cannot insert into foreign table \"%s\"",
> > RelationGetRelationName(resultRel))));
> >
> > Alternatively, we could just remove the mention about "if the FDW
> > supports it", since it's probably unlikely for an FDW not to support
> > INSERT.
>
> AFAIK, there's no special support in FDWs for "tuple moving" as such. The
> "if the FDW supports it" refers to the FDW's ability to handle tuple
> routing. Note that moving/re-routing involves calling
> ExecPrepareTupleRouting followed by ExecInsert on the new tupls after the
> old tuple is deleted. If an FDW doesn't support tuple routing, then a
> tuple cannot be moved into it. That's what that text is talking about.
>
> Maybe, we should reword it as "if the FDW supports tuple routing", so that
> a reader doesn't go looking around for "tuple moving support" in FDWs.
I think you missed my point. If there's no special support for "tuple
moving", as you say, then what help is it to tell the user "if the FDW
supports tuple routing"? The answer is, it's not any help. How would
the user check such a fact?
As far as I can tell, this is just the requirements as defined in
CheckValidResultRel() for CMD_INSERT. Fragments of which I pasted
above.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 2019/03/06 13:30, David Rowley wrote:
> On Wed, 6 Mar 2019 at 17:20, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> On 2019/03/06 12:47, David Rowley wrote:
>>> It seems a bit light on detail to me. If I was a user I'd want to know
>>> what exactly the FDW needed to support this. Does it need a special
>>> partition move function? Looking at ExecFindPartition(), this check
>>> seems to be done in CheckValidResultRel() and is basically:
>>>
>>> case RELKIND_FOREIGN_TABLE:
>>> /* Okay only if the FDW supports it */
>>> fdwroutine = resultRelInfo->ri_FdwRoutine;
>>> switch (operation)
>>> {
>>> case CMD_INSERT:
>>> if (fdwroutine->ExecForeignInsert == NULL)
>>> ereport(ERROR,
>>> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>> errmsg("cannot insert into foreign table \"%s\"",
>>> RelationGetRelationName(resultRel))));
>>>
>>> Alternatively, we could just remove the mention about "if the FDW
>>> supports it", since it's probably unlikely for an FDW not to support
>>> INSERT.
>>
>> AFAIK, there's no special support in FDWs for "tuple moving" as such. The
>> "if the FDW supports it" refers to the FDW's ability to handle tuple
>> routing. Note that moving/re-routing involves calling
>> ExecPrepareTupleRouting followed by ExecInsert on the new tupls after the
>> old tuple is deleted. If an FDW doesn't support tuple routing, then a
>> tuple cannot be moved into it. That's what that text is talking about.
>>
>> Maybe, we should reword it as "if the FDW supports tuple routing", so that
>> a reader doesn't go looking around for "tuple moving support" in FDWs.
>
> I think you missed my point. If there's no special support for "tuple
> moving", as you say, then what help is it to tell the user "if the FDW
> supports tuple routing"? The answer is, it's not any help. How would
> the user check such a fact?
Hmm, maybe getting the following error, like one would get in PG 10 when
using postgres_fdw-managed partitions:
ERROR: cannot route inserted tuples to a foreign table
Getting the above error is perhaps not the best way for a user to learn of
this fact, but maybe we (and hopefully other FDW authors) mention this in
the documentation?
> As far as I can tell, this is just the requirements as defined in
> CheckValidResultRel() for CMD_INSERT. Fragments of which I pasted
> above.
Only supporting INSERT doesn't suffice though. An FDW which intends to
support tuple routing and hence 1-way tuple moving needs to updated like
postgres_fdw was in PG 11.
Thanks,
Amit
(2019/03/06 13:18), Amit Langote wrote:
> The main problem here is indeed that the limitation is not listed under
> the partitioning limitations in ddl.sgml, where it's easier to notice than
> in the UPDATE's page.
Agreed.
> I've updated my patch to remove the release-11.sgml
> changes.
Thanks for the updated patch!
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3376,6 +3376,13 @@ ALTER TABLE measurement ATTACH PARTITION
measurement_y2008m02
</para>
</listitem>
+ <listitem>
+ <para>
+ <command>UPDATE</command> row movement is not supported in the cases
+ where the old row is contained in a foreign table partition.
+ </para>
+ </listitem>
ISTM that it's also a limitation that rows can be moved from a local
partition to a foreign partition *if the FDW support tuple routing*, so
I would vote for mentioning that as well here.
Best regards,
Etsuro Fujita
(2019/03/06 13:53), Amit Langote wrote: > On 2019/03/06 13:30, David Rowley wrote: >> I think you missed my point. If there's no special support for "tuple >> moving", as you say, then what help is it to tell the user "if the FDW >> supports tuple routing"? The answer is, it's not any help. How would >> the user check such a fact? > > Hmm, maybe getting the following error, like one would get in PG 10 when > using postgres_fdw-managed partitions: > > ERROR: cannot route inserted tuples to a foreign table > > Getting the above error is perhaps not the best way for a user to learn of > this fact, but maybe we (and hopefully other FDW authors) mention this in > the documentation? +1 >> As far as I can tell, this is just the requirements as defined in >> CheckValidResultRel() for CMD_INSERT. Fragments of which I pasted >> above. > > Only supporting INSERT doesn't suffice though. An FDW which intends to > support tuple routing and hence 1-way tuple moving needs to updated like > postgres_fdw was in PG 11. That's right; the "if the FDW supports it" in the documentation refers to the FDW's support for the callback functions BeginForeignInsert() and EndForeignInsert() described in 57.2.4. FDW Routines For Updating Foreign Tables [1] in addition to ExecForeignInsert(), as stated there: "Tuples inserted into a partitioned table by INSERT or COPY FROM are routed to partitions. If an FDW supports routable foreign-table partitions, it should also provide the following callback functions." Best regards, Etsuro Fujita [1] https://www.postgresql.org/docs/current/fdw-callbacks.html#FDW-CALLBACKS-UPDATE
Fujita-san, On 2019/03/06 15:10, Etsuro Fujita wrote: > --- a/doc/src/sgml/ddl.sgml > +++ b/doc/src/sgml/ddl.sgml > @@ -3376,6 +3376,13 @@ ALTER TABLE measurement ATTACH PARTITION > measurement_y2008m02 > </para> > </listitem> > > + <listitem> > + <para> > + <command>UPDATE</command> row movement is not supported in the cases > + where the old row is contained in a foreign table partition. > + </para> > + </listitem> > > ISTM that it's also a limitation that rows can be moved from a local > partition to a foreign partition *if the FDW support tuple routing*, so I > would vote for mentioning that as well here. Thanks for checking. I have updated the patch to include a line about this in the same paragraph, because maybe we don't need to make a new <listitem> for it. Thanks, Amit
Attachment
(2019/03/06 15:34), Amit Langote wrote: > On 2019/03/06 15:10, Etsuro Fujita wrote: >> --- a/doc/src/sgml/ddl.sgml >> +++ b/doc/src/sgml/ddl.sgml >> @@ -3376,6 +3376,13 @@ ALTER TABLE measurement ATTACH PARTITION >> measurement_y2008m02 >> </para> >> </listitem> >> >> +<listitem> >> +<para> >> +<command>UPDATE</command> row movement is not supported in the cases >> + where the old row is contained in a foreign table partition. >> +</para> >> +</listitem> >> >> ISTM that it's also a limitation that rows can be moved from a local >> partition to a foreign partition *if the FDW support tuple routing*, so I >> would vote for mentioning that as well here. > > Thanks for checking. > > I have updated the patch to include a line about this in the same > paragraph, because maybe we don't need to make a new<listitem> for it. Thanks for the patch! The patch looks good to me, but one thing I'm wondering is: as suggested by David, it would be better to rephrase this mention in the UPDATE reference page, in a single commit: "Currently, rows cannot be moved from a partition that is a foreign table to some other partition, but they can be moved into a foreign table if the foreign data wrapper supports it." I don't think it needs to be completely rephrased; it's enough for me to rewrite it to something like this: "Currently, rows cannot be moved from a foreign-table partition to some other partition, but they can be moved into a foreign-table partition if the foreign data wrapper supports tuple routing." And to make maintenance work easy, I think it might be better to just put this on the limitations section of 5.10. Table Partitioning. What do you think about that? Best regards, Etsuro Fujita
On Thu, Mar 7, 2019 at 7:35 AM Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote: > Thanks for the patch! > > The patch looks good to me, but one thing I'm wondering is: as suggested > by David, it would be better to rephrase this mention in the UPDATE > reference page, in a single commit: > > "Currently, rows cannot be moved from a partition that is a foreign > table to some other partition, but they can be moved into a foreign > table if the foreign data wrapper supports it." > > I don't think it needs to be completely rephrased; it's enough for me to > rewrite it to something like this: > > "Currently, rows cannot be moved from a foreign-table partition to some > other partition, but they can be moved into a foreign-table partition if > the foreign data wrapper supports tuple routing." I prefer David's wording. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2019/03/07 22:54, Robert Haas wrote: > On Thu, Mar 7, 2019 at 7:35 AM Etsuro Fujita > <fujita.etsuro@lab.ntt.co.jp> wrote: >> Thanks for the patch! >> >> The patch looks good to me, but one thing I'm wondering is: as suggested >> by David, it would be better to rephrase this mention in the UPDATE >> reference page, in a single commit: >> >> "Currently, rows cannot be moved from a partition that is a foreign >> table to some other partition, but they can be moved into a foreign >> table if the foreign data wrapper supports it." >> >> I don't think it needs to be completely rephrased; it's enough for me to >> rewrite it to something like this: >> >> "Currently, rows cannot be moved from a foreign-table partition to some >> other partition, but they can be moved into a foreign-table partition if >> the foreign data wrapper supports tuple routing." > > I prefer David's wording. IIUC, David's suggestion [1] is to change the existing wording, which he found hard to parse, to something like Fujita-san is suggesting. Thanks, Amit [1] https://www.postgresql.org/message-id/CAKJS1f-SauQJftjcaQ7C_tzHh_be5C8shT-E9qYnVp%2Bjh4-Fww%40mail.gmail.com
Thanks for the review. On 2019/03/07 21:35, Etsuro Fujita wrote: > The patch looks good to me, but one thing I'm wondering is: as suggested > by David, it would be better to rephrase this mention in the UPDATE > reference page, in a single commit: > > "Currently, rows cannot be moved from a partition that is a foreign table > to some other partition, but they can be moved into a foreign table if the > foreign data wrapper supports it." > > I don't think it needs to be completely rephrased; it's enough for me to > rewrite it to something like this: > > "Currently, rows cannot be moved from a foreign-table partition to some > other partition, but they can be moved into a foreign-table partition if > the foreign data wrapper supports tuple routing." > > And to make maintenance work easy, I think it might be better to just put > this on the limitations section of 5.10. Table Partitioning. What do you > think about that? I agree, so updated the patch this way. David, can you confirm if the rewritten text reads unambiguous or perhaps suggest a better wording? Thanks, Amit
Attachment
On Fri, 8 Mar 2019 at 15:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > David, can you confirm if the rewritten text reads unambiguous or perhaps > suggest a better wording? So this is the text: + Currently, rows cannot be moved from a foreign-table partition to some + other partition, but they can be moved into a foreign-table partition if + the foreign data wrapper supports tuple routing. I read this to mean that rows cannot normally be moved out of a foreign-table partition unless the new partition is a foreign one that uses an FDW that supports tuple routing. So let's test that: create extension postgres_fdw ; do $$ begin execute 'create server loopback foreign data wrapper postgres_fdw options (dbname ''' || current_database() || ''');'; end; $$; create user mapping for current_user server loopback; create table listp (a int) partition by list (a); create table listp1 (a int, check (a = 1)); create table listp2 (a int, check (a = 2)); create foreign table listpf1 partition of listp for values in (1) server loopback options (table_name 'listp1'); create foreign table listpf2 partition of listp for values in (2) server loopback options (table_name 'listp2'); insert into listp values (1); update listp set a = 2 where a = 1; ERROR: new row for relation "listp1" violates check constraint "listp1_a_check" DETAIL: Failing row contains (2). CONTEXT: remote SQL command: UPDATE public.listp1 SET a = 2 WHERE ((a = 1)) I'd be filing a bug report for that as I'm moving a row into a foreign table with an FDW that supports tuple routing. Where I think you're going wrong is, in one part of the sentence you're talking about UPDATE, then in the next part you seem to magically jump to talking about INSERT. Since the entire paragraph is talking about UPDATE, why is it relevant to talk about INSERT? I thought my doc_confirm_foreign_partition_limitations.patch had this pretty clear with: + <listitem> + <para> + Currently, an <command>UPDATE</command> of a partitioned table cannot + move rows out of a foreign partition into another partition. + </para> + </listitem> Or is my understanding of this incorrect? I also think the new paragraph is a good move as it's a pretty restrictive limitation for anyone that wants to set up a partition hierarchy with foreign partitions. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
(2019/03/08 19:29), David Rowley wrote: > On Fri, 8 Mar 2019 at 15:07, Amit Langote<Langote_Amit_f8@lab.ntt.co.jp> wrote: >> David, can you confirm if the rewritten text reads unambiguous or perhaps >> suggest a better wording? > > So this is the text: > > + Currently, rows cannot be moved from a foreign-table partition to some > + other partition, but they can be moved into a foreign-table partition if > + the foreign data wrapper supports tuple routing. > > I read this to mean that rows cannot normally be moved out of a > foreign-table partition unless the new partition is a foreign one that > uses an FDW that supports tuple routing. > > So let's test that: > > create extension postgres_fdw ; > do $$ begin execute 'create server loopback foreign data wrapper > postgres_fdw options (dbname ''' || current_database() || ''');'; end; > $$; > create user mapping for current_user server loopback; > > create table listp (a int) partition by list (a); > create table listp1 (a int, check (a = 1)); > create table listp2 (a int, check (a = 2)); > > create foreign table listpf1 partition of listp for values in (1) > server loopback options (table_name 'listp1'); > create foreign table listpf2 partition of listp for values in (2) > server loopback options (table_name 'listp2'); > > insert into listp values (1); > > update listp set a = 2 where a = 1; > ERROR: new row for relation "listp1" violates check constraint "listp1_a_check" > DETAIL: Failing row contains (2). > CONTEXT: remote SQL command: UPDATE public.listp1 SET a = 2 WHERE ((a = 1)) > > I'd be filing a bug report for that as I'm moving a row into a foreign > table with an FDW that supports tuple routing. Fair enough. > Where I think you're going wrong is, in one part of the sentence > you're talking about UPDATE, then in the next part you seem to > magically jump to talking about INSERT. Since the entire paragraph is > talking about UPDATE, why is it relevant to talk about INSERT? > > I thought my doc_confirm_foreign_partition_limitations.patch had this > pretty clear with: > > +<listitem> > +<para> > + Currently, an<command>UPDATE</command> of a partitioned table cannot > + move rows out of a foreign partition into another partition. > +</para> > +</listitem> > > Or is my understanding of this incorrect? IMO I think it's better that we also mention that the UPDATE can move rows into a foreign partition if the FDW supports it. No? > I also think the new > paragraph is a good move as it's a pretty restrictive limitation for > anyone that wants to set up a partition hierarchy with foreign > partitions. +1 Best regards, Etsuro Fujita
On Sat, 9 Mar 2019 at 00:09, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote: > IMO I think it's better that we also mention that the UPDATE can move > rows into a foreign partition if the FDW supports it. No? In my opinion, there's not much need to talk about what the limitations are not when you're mentioning what the limitations are. Maybe it would be worth it if the text was slightly unclear on what's affected, but I thought my version was fairly clear. If you think that it's still unclear, then I wouldn't object to adding " There is no such restriction on <command>UPDATE</command> row movements out of native partitions into foreign ones.". Obviously, it's got to be clear for everyone, not just the person who wrote it. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Mar 8, 2019 at 8:21 PM David Rowley
<david.rowley@2ndquadrant.com> wrote:
>
> On Sat, 9 Mar 2019 at 00:09, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
> > IMO I think it's better that we also mention that the UPDATE can move
> > rows into a foreign partition if the FDW supports it. No?
>
> In my opinion, there's not much need to talk about what the
> limitations are not when you're mentioning what the limitations are.
In this case, I think it might be OK to contrast the two cases so that
it's clear exactly which side doesn't work and which works. We also
have the following text in limitations:
<listitem>
<para>
While primary keys are supported on partitioned tables, foreign
keys referencing partitioned tables are not supported. (Foreign key
references from a partitioned table to some other table are supported.)
</para>
</listitem>
> Maybe it would be worth it if the text was slightly unclear on what's
> affected, but I thought my version was fairly clear.
>
> If you think that it's still unclear, then I wouldn't object to adding
> " There is no such restriction on <command>UPDATE</command> row
> movements out of native partitions into foreign ones.". Obviously,
> it's got to be clear for everyone, not just the person who wrote it.
OK, how about this:
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3877,6 +3877,15 @@ ALTER TABLE measurement ATTACH PARTITION
measurement_y2008m02
</para>
</listitem>
+ <listitem>
+ <para>
+ While rows can be moved from local partitions to a foreign-table
+ partition (provided the foreign data wrapper supports tuple routing),
+ they cannot be moved from a foreign-table partition to some
+ other partition.
+ </para>
+ </listitem>
+
<listitem>
<para>
<literal>BEFORE ROW</literal> triggers, if necessary, must be defined
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 77430a586c..f5cf8eab85 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -291,9 +291,9 @@ UPDATE <replaceable class="parameter">count</replaceable>
concurrent <command>UPDATE</command> or <command>DELETE</command> on the
same row may miss this row. For details see the section
<xref linkend="ddl-partitioning-declarative-limitations"/>.
- Currently, rows cannot be moved from a partition that is a
- foreign table to some other partition, but they can be moved into a foreign
- table if the foreign data wrapper supports it.
+ While rows can be moved from local partitions to a foreign-table partition
+ partition (provided the foreign data wrapper supports tuple routing), they
+ cannot be moved from a foreign-table partition to some other partition.
</para>
</refsect1>
At least in update.sgml, we describe that row movement is implemented
as DELETE+INSERT, so I think maybe it's OK to mention tuple routing
when mentioning why that 1-way movement works. If someone's using an
FDW that doesn't support tuple routing to begin with, they'll be get
an error when trying to move rows from a local partition to a foreign
table partition using that FDW, which is this:
ERROR: cannot route inserted tuples to a foreign table
Then maybe they will come back to the read limitations and see why the
tuple movement didn't work.
Thanks,
Amit
Attachment
On 2019-Mar-08, Amit Langote wrote: > diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml > index 77430a586c..f5cf8eab85 100644 > --- a/doc/src/sgml/ref/update.sgml > +++ b/doc/src/sgml/ref/update.sgml > @@ -291,9 +291,9 @@ UPDATE <replaceable class="parameter">count</replaceable> > concurrent <command>UPDATE</command> or <command>DELETE</command> on the > same row may miss this row. For details see the section > <xref linkend="ddl-partitioning-declarative-limitations"/>. > - Currently, rows cannot be moved from a partition that is a > - foreign table to some other partition, but they can be moved into a foreign > - table if the foreign data wrapper supports it. > + While rows can be moved from local partitions to a foreign-table partition > + partition (provided the foreign data wrapper supports tuple routing), they > + cannot be moved from a foreign-table partition to some other partition. > </para> > </refsect1> LGTM. Maybe I'd change "some other" to "another", but maybe on a different phase of the moon I'd leave it alone. I'm not sure about copying the same to ddl.sgml. Why is that needed? Update is not DDL. ddl.sgml does say this: "Partitions can also be foreign tables, although they have some limitations that normal tables do not; see CREATE FOREIGN TABLE for more information." which suggests that the limitation might need to be added to create_foreign_table.sgml. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 8, 2019 at 11:09 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Mar-08, Amit Langote wrote: > > > diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml > > index 77430a586c..f5cf8eab85 100644 > > --- a/doc/src/sgml/ref/update.sgml > > +++ b/doc/src/sgml/ref/update.sgml > > @@ -291,9 +291,9 @@ UPDATE <replaceable class="parameter">count</replaceable> > > concurrent <command>UPDATE</command> or <command>DELETE</command> on the > > same row may miss this row. For details see the section > > <xref linkend="ddl-partitioning-declarative-limitations"/>. > > - Currently, rows cannot be moved from a partition that is a > > - foreign table to some other partition, but they can be moved into a foreign > > - table if the foreign data wrapper supports it. > > + While rows can be moved from local partitions to a foreign-table partition > > + partition (provided the foreign data wrapper supports tuple routing), they > > + cannot be moved from a foreign-table partition to some other partition. > > </para> > > </refsect1> > > LGTM. Maybe I'd change "some other" to "another", but maybe on a > different phase of the moon I'd leave it alone. Done. > I'm not sure about copying the same to ddl.sgml. Why is that needed? > Update is not DDL. Hmm, maybe because there's already a huge block of text describing certain limitations of UPDATE row movement under concurrency? Actually, I remember commenting *against* having that text in ddl.sgml, but it got in there anyway. > ddl.sgml does say this: "Partitions can also be > foreign tables, although they have some limitations that normal tables > do not; see CREATE FOREIGN TABLE for more information." which suggests > that the limitation might need to be added to create_foreign_table.sgml. Actually, that "more information" never got added to create_foreign_table.sgml. There should've been some text about the lack for tuple routing at least in PG 10's docs, but I guess that never happened. Should we start now by listing this UPDATE row movement limitation? Anyway, I've only attached the patch for update.sgml this time. Thanks, Amit
Attachment
On 2019-Mar-08, Amit Langote wrote: > On Fri, Mar 8, 2019 at 11:09 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > I'm not sure about copying the same to ddl.sgml. Why is that needed? > > Update is not DDL. > > Hmm, maybe because there's already a huge block of text describing > certain limitations of UPDATE row movement under concurrency? Uh, you're right, there is. That seems misplaced :-( I'm not sure it even counts as a "limitation"; it seems to belong to the NOTES section of UPDATE rather than where it is now. > Actually, I remember commenting *against* having that text in > ddl.sgml, but it got in there anyway. We can move it now ... > > ddl.sgml does say this: "Partitions can also be > > foreign tables, although they have some limitations that normal tables > > do not; see CREATE FOREIGN TABLE for more information." which suggests > > that the limitation might need to be added to create_foreign_table.sgml. > > Actually, that "more information" never got added to > create_foreign_table.sgml. There should've been some text about the > lack for tuple routing at least in PG 10's docs, but I guess that > never happened. Sigh. Since version 10 is going to be supported for a few years still, maybe we should add it there. > Should we start now by listing this UPDATE row movement limitation? I think we should, yes. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Mar 9, 2019 at 12:03 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Mar-08, Amit Langote wrote: > > > On Fri, Mar 8, 2019 at 11:09 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > I'm not sure about copying the same to ddl.sgml. Why is that needed? > > > Update is not DDL. > > > > Hmm, maybe because there's already a huge block of text describing > > certain limitations of UPDATE row movement under concurrency? > > Uh, you're right, there is. That seems misplaced :-( I'm not sure it > even counts as a "limitation"; it seems to belong to the NOTES section > of UPDATE rather than where it is now. > > > Actually, I remember commenting *against* having that text in > > ddl.sgml, but it got in there anyway. > > We can move it now ... > > > > ddl.sgml does say this: "Partitions can also be > > > foreign tables, although they have some limitations that normal tables > > > do not; see CREATE FOREIGN TABLE for more information." which suggests > > > that the limitation might need to be added to create_foreign_table.sgml. > > > > Actually, that "more information" never got added to > > create_foreign_table.sgml. There should've been some text about the > > lack for tuple routing at least in PG 10's docs, but I guess that > > never happened. > > Sigh. > > Since version 10 is going to be supported for a few years still, maybe > we should add it there. > > > Should we start now by listing this UPDATE row movement limitation? > > I think we should, yes. Attached find 3 patches -- for PG 10, 11, and HEAD. I also realizes that a description of PARTITION OF clause was also missing in the Parameters section of CREATE FOREIGN TABLE, which is fixed too. Thanks, Amit
Attachment
On 2019-Mar-09, Amit Langote wrote: > Attached find 3 patches -- for PG 10, 11, and HEAD. I also realizes > that a description of PARTITION OF clause was also missing in the > Parameters section of CREATE FOREIGN TABLE, which is fixed too. Thanks! Applied all three -- I appreciate your help in getting this sorted out. (There were a number of xml/sgml errors, plus one typo, though.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
As the original reporter, thanks a ton for all the hard work you're putting into the documentation!
On Tue, Mar 12, 2019, 12:04 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Mar-09, Amit Langote wrote:
> Attached find 3 patches -- for PG 10, 11, and HEAD. I also realizes
> that a description of PARTITION OF clause was also missing in the
> Parameters section of CREATE FOREIGN TABLE, which is fixed too.
Thanks! Applied all three -- I appreciate your help in getting this
sorted out. (There were a number of xml/sgml errors, plus one typo,
though.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019/03/13 1:04, Alvaro Herrera wrote: > On 2019-Mar-09, Amit Langote wrote: > >> Attached find 3 patches -- for PG 10, 11, and HEAD. I also realizes >> that a description of PARTITION OF clause was also missing in the >> Parameters section of CREATE FOREIGN TABLE, which is fixed too. > > Thanks! Applied all three -- I appreciate your help in getting this > sorted out. (There were a number of xml/sgml errors, plus one typo, > though.) Ah, thanks for fixing and committing. Regards, Amit