Обсуждение: [HACKERS] Tuple-routing for certain partitioned tables not working as expected

Hi,

I noticed that tuple-routing for partitioned tables that contain 
non-writable foreign partitions doesn't work as expected.  Here is an 
example:

postgres=# create extension file_fdw;
postgres=# create server file_server foreign data wrapper file_fdw;
postgres=# create user mapping for CURRENT_USER server file_server;
postgres=# create table p (a int) partition by list (a);
postgres=# create foreign table t1 partition of p for values in (1) 
server file_server options (format 'csv', filename '/path/to/file', 
delimiter ',');
postgres=# create table t2 partition of p for values in (2);
postgres=# insert into p values (1);
ERROR:  cannot insert into foreign table "t1"

Looks good, but:

postgres=# insert into p values (2);
ERROR:  cannot insert into foreign table "t1"

The insert should work but doesn't.  (It also seems odd to me that the 
error message points to t1, not t2.)  The reason for that is 
CheckValidResultRel in ExecSetupPartitionTupleRouting aborts any insert 
into a partitioned table in the case where the partitioned table 
contains at least one foreign partition into which the FDW can't insert. 
  I don't think that that is intentional behavior, so I'd like to 
propose to fix that by skipping CheckValidResultRel for foreign 
partitions because we can abort an insert into a foreign partition after 
ExecFindPartition in ExecInsert, by checking to see if 
resultRelInfo->ri_FdwRoutine is not NULL.  Attached is a proposed patch 
for that.  Since COPY FROM has the same issue, I added regression tests 
for COPY FROM as well as INSERT to file_fdw.

Best regards,
Etsuro Fujita

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fujita-san,

On 2017/08/07 12:45, Etsuro Fujita wrote:
> Hi,
> 
> I noticed that tuple-routing for partitioned tables that contain
> non-writable foreign partitions doesn't work as expected.  Here is an
> example:
> 
> postgres=# create extension file_fdw;
> postgres=# create server file_server foreign data wrapper file_fdw;
> postgres=# create user mapping for CURRENT_USER server file_server;
> postgres=# create table p (a int) partition by list (a);
> postgres=# create foreign table t1 partition of p for values in (1) server
> file_server options (format 'csv', filename '/path/to/file', delimiter ',');
> postgres=# create table t2 partition of p for values in (2);
> postgres=# insert into p values (1);
> ERROR:  cannot insert into foreign table "t1"
> 
> Looks good, but:
> 
> postgres=# insert into p values (2);
> ERROR:  cannot insert into foreign table "t1"
> 
> The insert should work but doesn't.  (It also seems odd to me that the
> error message points to t1, not t2.)  The reason for that is
> CheckValidResultRel in ExecSetupPartitionTupleRouting aborts any insert
> into a partitioned table in the case where the partitioned table contains
> at least one foreign partition into which the FDW can't insert.  I don't
> think that that is intentional behavior, so I'd like to propose to fix
> that by skipping CheckValidResultRel for foreign partitions because we can
> abort an insert into a foreign partition after ExecFindPartition in
> ExecInsert, by checking to see if resultRelInfo->ri_FdwRoutine is not
> NULL.  Attached is a proposed patch for that.  Since COPY FROM has the
> same issue, I added regression tests for COPY FROM as well as INSERT to
> file_fdw.

Thanks for reporting this.  All the issues you mention here seem valid to me.

So, once we add support in general for foreign partition tuple-routing,
we'd still get the same error ("cannot route to foreign partitions") in
the file_fdw's case, but only because
resultRelInfo->ri_FdwRoutine->ExecForeignInsert will be NULL for file_fdw.

The patch looks good too.  Although, looking at the following hunk:

+         Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
+                partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
+         /*          * Verify result relation is a valid target for the current operation.          */
!         if (partrel->rd_rel->relkind == RELKIND_RELATION)
!             CheckValidResultRel(partrel, CMD_INSERT);

makes me now wonder if we need the CheckValidResultRel check at all.  The
only check currently in place for RELKIND_RELATION is
CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts anyway.

Thanks,
Amit

On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too. 
Although, looking at the following hunk:
> 
> +         Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
> +                partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
> +
>            /*
>             * Verify result relation is a valid target for the current operation.
>             */
> !         if (partrel->rd_rel->relkind == RELKIND_RELATION)
> !             CheckValidResultRel(partrel, CMD_INSERT);
> 
> makes me now wonder if we need the CheckValidResultRel check at all.  The
> only check currently in place for RELKIND_RELATION is
> CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts anyway.

Good point!  I left the verification for a plain table because that is 
harmless but as you mentioned, that is nothing but an overhead.  So, 
here is a new version which removes the verification at all from 
ExecSetupPartitionTupleRouting.

Best regards,
Etsuro Fujita

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/08/07 15:22, Etsuro Fujita wrote:
> On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too.
> Although, looking at the following hunk:
>>
>> +         Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
>> +                partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
>> +
>>            /*
>>             * Verify result relation is a valid target for the current
>> operation.
>>             */
>> !         if (partrel->rd_rel->relkind == RELKIND_RELATION)
>> !             CheckValidResultRel(partrel, CMD_INSERT);
>>
>> makes me now wonder if we need the CheckValidResultRel check at all.  The
>> only check currently in place for RELKIND_RELATION is
>> CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts
>> anyway.
> 
> Good point!  I left the verification for a plain table because that is
> harmless but as you mentioned, that is nothing but an overhead.  So, here
> is a new version which removes the verification at all from
> ExecSetupPartitionTupleRouting.

The updated patch looks good to me, thanks.

Regards,
Amit

On 2017/08/07 15:33, Amit Langote wrote:
> On 2017/08/07 15:22, Etsuro Fujita wrote:
>> On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too.
>> Although, looking at the following hunk:
>>>
>>> +         Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
>>> +                partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
>>> +
>>>             /*
>>>              * Verify result relation is a valid target for the current
>>> operation.
>>>              */
>>> !         if (partrel->rd_rel->relkind == RELKIND_RELATION)
>>> !             CheckValidResultRel(partrel, CMD_INSERT);
>>>
>>> makes me now wonder if we need the CheckValidResultRel check at all.  The
>>> only check currently in place for RELKIND_RELATION is
>>> CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts
>>> anyway.
>>
>> Good point!  I left the verification for a plain table because that is
>> harmless but as you mentioned, that is nothing but an overhead.  So, here
>> is a new version which removes the verification at all from
>> ExecSetupPartitionTupleRouting.
> 
> The updated patch looks good to me, thanks.

Thanks for the review!

Best regards,
Etsuro Fujita

On 2017/08/07 15:45, Etsuro Fujita wrote:
> On 2017/08/07 15:33, Amit Langote wrote:
>> On 2017/08/07 15:22, Etsuro Fujita wrote:
>>> On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too.
>>> Although, looking at the following hunk:
>>>>
>>>> +         Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
>>>> +                partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
>>>> +
>>>>             /*
>>>>              * Verify result relation is a valid target for the current
>>>> operation.
>>>>              */
>>>> !         if (partrel->rd_rel->relkind == RELKIND_RELATION)
>>>> !             CheckValidResultRel(partrel, CMD_INSERT);
>>>>
>>>> makes me now wonder if we need the CheckValidResultRel check at 
>>>> all.  The
>>>> only check currently in place for RELKIND_RELATION is
>>>> CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts
>>>> anyway.
>>>
>>> Good point!  I left the verification for a plain table because that is
>>> harmless but as you mentioned, that is nothing but an overhead.  So, 
>>> here
>>> is a new version which removes the verification at all from
>>> ExecSetupPartitionTupleRouting.
>>
>> The updated patch looks good to me, thanks.
> 
> Thanks for the review!

If there are no objections, I'll add this to the open item list for v10.

Best regards,
Etsuro Fujita

On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> If there are no objections, I'll add this to the open item list for v10.

This seems fairly ad-hoc to me.  I mean, now you have
CheckValidResultRel not being called in just this one case -- but that
bypasses all the checks that function might do, not just this one.  It
so happens that's OK at the moment because CheckCmdReplicaIdentity()
doesn't do anything in the insert case.

I'm somewhat inclined to just view this as a limitation of v10 and fix
it in v11.  If you want to fix it in v10, I think we need a different
approach -- just ripping the CheckValidResultRel checks out entirely
doesn't seem like a good idea to me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017/08/22 1:08, Robert Haas wrote:
> On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> If there are no objections, I'll add this to the open item list for v10.
> 
> This seems fairly ad-hoc to me.  I mean, now you have
> CheckValidResultRel not being called in just this one case -- but that
> bypasses all the checks that function might do, not just this one.  It
> so happens that's OK at the moment because CheckCmdReplicaIdentity()
> doesn't do anything in the insert case.
>
> I'm somewhat inclined to just view this as a limitation of v10 and fix
> it in v11.  If you want to fix it in v10, I think we need a different
> approach -- just ripping the CheckValidResultRel checks out entirely
> doesn't seem like a good idea to me.

Before 389af951552f, the relkind check that is now performed by
CheckValidResultRel(), used to be done in InitResultRelInfo().  ISTM, it
was separated out so that certain ResultRelInfos could be initialized
without the explicit relkind check, either because it's taken care of
elsewhere or the table in question is *known* to be a valid result
relation.  Maybe, mostly just the former of the two reasons when that
commit went in.

IMO, the latter case applies when initializing ResultRelInfos for
partitions during tuple-routing, because the table types we allow to
become partitions are fairly restricted.

Also, it seems okay to show the error messages that CheckValidResultRel()
shows when the concerned table is *directly* addressed in a query, but the
same error does not seem so user-friendly when emitted for one of the
partitions while tuple-routing is being set up.  IMHO, there should be
"tuple routing" somewhere in the error message shown in that case, even if
it's for the total lack of support for inserts by a FDW.

Thanks,
Amit

On 2017/08/22 9:55, Amit Langote wrote:
> On 2017/08/22 1:08, Robert Haas wrote:
>> On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita
>> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>> If there are no objections, I'll add this to the open item list for v10.
>>
>> This seems fairly ad-hoc to me.  I mean, now you have
>> CheckValidResultRel not being called in just this one case -- but that
>> bypasses all the checks that function might do, not just this one.  It
>> so happens that's OK at the moment because CheckCmdReplicaIdentity()
>> doesn't do anything in the insert case.
>>
>> I'm somewhat inclined to just view this as a limitation of v10 and fix
>> it in v11.

That limitation seems too restrictive to me.

>> If you want to fix it in v10, I think we need a different
>> approach -- just ripping the CheckValidResultRel checks out entirely
>> doesn't seem like a good idea to me.
> 
> Before 389af951552f, the relkind check that is now performed by
> CheckValidResultRel(), used to be done in InitResultRelInfo().  ISTM, it
> was separated out so that certain ResultRelInfos could be initialized
> without the explicit relkind check, either because it's taken care of
> elsewhere or the table in question is *known* to be a valid result
> relation.  Maybe, mostly just the former of the two reasons when that
> commit went in.
> 
> IMO, the latter case applies when initializing ResultRelInfos for
> partitions during tuple-routing, because the table types we allow to
> become partitions are fairly restricted.

Agreed.

> Also, it seems okay to show the error messages that CheckValidResultRel()
> shows when the concerned table is *directly* addressed in a query, but the
> same error does not seem so user-friendly when emitted for one of the
> partitions while tuple-routing is being set up.  IMHO, there should be
> "tuple routing" somewhere in the error message shown in that case, even if
> it's for the total lack of support for inserts by a FDW.

Agreed, but I'd vote for fixing this in v10 as proposed; I agree that 
just ripping the CheckValidResultRel checks out entirely is not a good 
idea, but that seems OK to me at least as a fix just for v10.

Best regards,
Etsuro Fujita

On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
> ripping the CheckValidResultRel checks out entirely is not a good idea, but
> that seems OK to me at least as a fix just for v10.

I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel.  If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.

At some point we've got to stop developing v10 and just let it be what it is.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017/08/25 22:26, Robert Haas wrote:
> On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
>> ripping the CheckValidResultRel checks out entirely is not a good idea, but
>> that seems OK to me at least as a fix just for v10.
> 
> I'm still not on-board with having this be the one case where we don't
> do CheckValidResultRel.  If we want to still call it but pass down
> some additional information that can selectively skip certain checks,
> I could probably live with that.

Another idea would be to not do CheckValidResultRel for partitions in 
ExecSetupPartitionTupleRouting; instead, do that the first time the 
partition is chosen by ExecFindPartition, and if successfully checked, 
initialize the partition's ResultRelInfo and other stuff.  (We could 
skip this after the first time by checking whether we already have a 
valid ResultRelInfo for the chosen partition.)  That could allow us to 
not only call CheckValidResultRel the way it is, but avoid initializing 
useless partitions for which tuples aren't routed at all.

> At some point we've got to stop developing v10 and just let it be what it is.

I agree on that point, but ISTM that this is more like a bug.

Best regards,
Etsuro Fujita

On 2017/08/29 20:18, Etsuro Fujita wrote:
> On 2017/08/25 22:26, Robert Haas wrote:
>> On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
>> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>> Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
>>> ripping the CheckValidResultRel checks out entirely is not a good idea,
>>> but
>>> that seems OK to me at least as a fix just for v10.
>>
>> I'm still not on-board with having this be the one case where we don't
>> do CheckValidResultRel.  If we want to still call it but pass down
>> some additional information that can selectively skip certain checks,
>> I could probably live with that.
> 
> Another idea would be to not do CheckValidResultRel for partitions in
> ExecSetupPartitionTupleRouting; instead, do that the first time the
> partition is chosen by ExecFindPartition, and if successfully checked,
> initialize the partition's ResultRelInfo and other stuff.  (We could skip
> this after the first time by checking whether we already have a valid
> ResultRelInfo for the chosen partition.)  That could allow us to not only
> call CheckValidResultRel the way it is, but avoid initializing useless
> partitions for which tuples aren't routed at all.

I too have thought about the idea of lazy initialization of the partition
ResultRelInfos.  I think it would be a good idea, but definitely something
for PG 11.

Thanks,
Amit

On 2017/08/30 9:13, Amit Langote wrote:
> On 2017/08/29 20:18, Etsuro Fujita wrote:
>> On 2017/08/25 22:26, Robert Haas wrote:
>>> On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
>>> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>>> Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
>>>> ripping the CheckValidResultRel checks out entirely is not a good idea,
>>>> but
>>>> that seems OK to me at least as a fix just for v10.
>>>
>>> I'm still not on-board with having this be the one case where we don't
>>> do CheckValidResultRel.  If we want to still call it but pass down
>>> some additional information that can selectively skip certain checks,
>>> I could probably live with that.
>>
>> Another idea would be to not do CheckValidResultRel for partitions in
>> ExecSetupPartitionTupleRouting; instead, do that the first time the
>> partition is chosen by ExecFindPartition, and if successfully checked,
>> initialize the partition's ResultRelInfo and other stuff.  (We could skip
>> this after the first time by checking whether we already have a valid
>> ResultRelInfo for the chosen partition.)  That could allow us to not only
>> call CheckValidResultRel the way it is, but avoid initializing useless
>> partitions for which tuples aren't routed at all.
> 
> I too have thought about the idea of lazy initialization of the partition
> ResultRelInfos.  I think it would be a good idea, but definitely something
> for PG 11.

Agreed.  Here is a patch to skip the CheckValidResultRel checks for a 
target table if it's a foreign partition to perform tuple-routing for, 
as proposed by Robert.

Best regards,
Etsuro Fujita

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/08/30 17:20, Etsuro Fujita wrote:
> On 2017/08/30 9:13, Amit Langote wrote:
>> On 2017/08/29 20:18, Etsuro Fujita wrote:
>>> On 2017/08/25 22:26, Robert Haas wrote:
>>>> On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
>>>> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>>>> Agreed, but I'd vote for fixing this in v10 as proposed; I agree 
>>>>> that just
>>>>> ripping the CheckValidResultRel checks out entirely is not a good 
>>>>> idea,
>>>>> but
>>>>> that seems OK to me at least as a fix just for v10.
>>>>
>>>> I'm still not on-board with having this be the one case where we don't
>>>> do CheckValidResultRel.  If we want to still call it but pass down
>>>> some additional information that can selectively skip certain checks,
>>>> I could probably live with that.
>>>
>>> Another idea would be to not do CheckValidResultRel for partitions in
>>> ExecSetupPartitionTupleRouting; instead, do that the first time the
>>> partition is chosen by ExecFindPartition, and if successfully checked,
>>> initialize the partition's ResultRelInfo and other stuff.  (We could 
>>> skip
>>> this after the first time by checking whether we already have a valid
>>> ResultRelInfo for the chosen partition.)  That could allow us to not 
>>> only
>>> call CheckValidResultRel the way it is, but avoid initializing useless
>>> partitions for which tuples aren't routed at all.
>>
>> I too have thought about the idea of lazy initialization of the partition
>> ResultRelInfos.  I think it would be a good idea, but definitely 
>> something
>> for PG 11.
> 
> Agreed.  Here is a patch to skip the CheckValidResultRel checks for a 
> target table if it's a foreign partition to perform tuple-routing for, 
> as proposed by Robert.

I'll add this to the open items list for v10.

Best regards,
Etsuro Fujita

On Tue, Sep 05, 2017 at 08:35:13PM +0900, Etsuro Fujita wrote:
> On 2017/08/30 17:20, Etsuro Fujita wrote:
> >On 2017/08/30 9:13, Amit Langote wrote:
> >>On 2017/08/29 20:18, Etsuro Fujita wrote:
> >>>On 2017/08/25 22:26, Robert Haas wrote:
> >>>>On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
> >>>><fujita.etsuro@lab.ntt.co.jp> wrote:
> >>>>>Agreed, but I'd vote for fixing this in v10 as proposed; I agree
> >>>>>that just
> >>>>>ripping the CheckValidResultRel checks out entirely is not a good
> >>>>>idea,
> >>>>>but
> >>>>>that seems OK to me at least as a fix just for v10.
> >>>>
> >>>>I'm still not on-board with having this be the one case where we don't
> >>>>do CheckValidResultRel.  If we want to still call it but pass down
> >>>>some additional information that can selectively skip certain checks,
> >>>>I could probably live with that.
> >>>
> >>>Another idea would be to not do CheckValidResultRel for partitions in
> >>>ExecSetupPartitionTupleRouting; instead, do that the first time the
> >>>partition is chosen by ExecFindPartition, and if successfully checked,
> >>>initialize the partition's ResultRelInfo and other stuff.  (We could
> >>>skip
> >>>this after the first time by checking whether we already have a valid
> >>>ResultRelInfo for the chosen partition.)  That could allow us to not
> >>>only
> >>>call CheckValidResultRel the way it is, but avoid initializing useless
> >>>partitions for which tuples aren't routed at all.
> >>
> >>I too have thought about the idea of lazy initialization of the partition
> >>ResultRelInfos.  I think it would be a good idea, but definitely
> >>something
> >>for PG 11.
> >
> >Agreed.  Here is a patch to skip the CheckValidResultRel checks for a
> >target table if it's a foreign partition to perform tuple-routing for, as
> >proposed by Robert.
> 
> I'll add this to the open items list for v10.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

On 2017/08/30 17:20, Etsuro Fujita wrote:
> Here is a patch to skip the CheckValidResultRel checks for a 
> target table if it's a foreign partition to perform tuple-routing for, 
> as proposed by Robert.

In the patch, to skip the checks, I passed to CheckValidResultRel a new 
flag indicating whether the target relation is a partition to do 
tuple-routing for, but while updating another patch for tuple-routing 
for foreign partitions in PG11, I noticed that it would be better to 
pass ResultRelInfo than that flag.  The reason is: (1) we can see 
whether the relation is such a partition by looking at the 
ResultRelInfo's ri_PartitionRoot, instead of that flag, and (2) this is 
for tuple-routing for foreign partitions, but we could extend that 
function with that argument to do the checks without throwing an error 
and save the result in a new member of ResultRelInfo (eg, 
ri_PartitionIsValid) so that we can use that info to determine in 
ExecInsert whether a foreign partition chosen by ExecFindPartition is 
valid for tuple-routing.  So, I updated the patch that way.  Attached is 
an updated version of the patch.

(I discussed about lazy initialization of partition ResultRelInfos 
before, but I changed my mind because I noticed that the change to 
EXPLAIN for INSERT into a partitioned table, which I'd like to propose 
in the tuple-routing-for-foereign-partitions patch, needs partition 
ResultRelInfos.)

Best regards,
Etsuro Fujita

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Sep 7, 2017 at 5:13 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2017/08/30 17:20, Etsuro Fujita wrote:
>> Here is a patch to skip the CheckValidResultRel checks for a target table
>> if it's a foreign partition to perform tuple-routing for, as proposed by
>> Robert.
>
> In the patch, to skip the checks, I passed to CheckValidResultRel a new flag
> indicating whether the target relation is a partition to do tuple-routing
> for, but while updating another patch for tuple-routing for foreign
> partitions in PG11, I noticed that it would be better to pass ResultRelInfo
> than that flag.  The reason is: (1) we can see whether the relation is such
> a partition by looking at the ResultRelInfo's ri_PartitionRoot, instead of
> that flag, and (2) this is for tuple-routing for foreign partitions, but we
> could extend that function with that argument to do the checks without
> throwing an error and save the result in a new member of ResultRelInfo (eg,
> ri_PartitionIsValid) so that we can use that info to determine in ExecInsert
> whether a foreign partition chosen by ExecFindPartition is valid for
> tuple-routing.  So, I updated the patch that way.  Attached is an updated
> version of the patch.

OK, committed to master and v10.  I am less convinced than you that
this was a must-fix issue, but it's not a very invasive change the way
you did it here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017/09/08 0:21, Robert Haas wrote:
> On Thu, Sep 7, 2017 at 5:13 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> On 2017/08/30 17:20, Etsuro Fujita wrote:
>>> Here is a patch to skip the CheckValidResultRel checks for a target table
>>> if it's a foreign partition to perform tuple-routing for, as proposed by
>>> Robert.
>>
>> In the patch, to skip the checks, I passed to CheckValidResultRel a new flag
>> indicating whether the target relation is a partition to do tuple-routing
>> for, but while updating another patch for tuple-routing for foreign
>> partitions in PG11, I noticed that it would be better to pass ResultRelInfo
>> than that flag.  The reason is: (1) we can see whether the relation is such
>> a partition by looking at the ResultRelInfo's ri_PartitionRoot, instead of
>> that flag, and (2) this is for tuple-routing for foreign partitions, but we
>> could extend that function with that argument to do the checks without
>> throwing an error and save the result in a new member of ResultRelInfo (eg,
>> ri_PartitionIsValid) so that we can use that info to determine in ExecInsert
>> whether a foreign partition chosen by ExecFindPartition is valid for
>> tuple-routing.  So, I updated the patch that way.  Attached is an updated
>> version of the patch.
> 
> OK, committed to master and v10.  I am less convinced than you that
> this was a must-fix issue, but it's not a very invasive change the way
> you did it here.

Thank you!

I'll update the tuple-routing-for-foreign-partitions patch that way.

Best regards,
Etsuro Fujita



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers