Обсуждение: [HACKERS] Tuple-routing for certain partitioned tables not working as expected
[HACKERS] Tuple-routing for certain partitioned tables not working as expected
От
Etsuro Fujita
Дата:
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
Вложения
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Amit Langote
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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
Вложения
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Amit Langote
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Robert Haas
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Amit Langote
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Robert Haas
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Amit Langote
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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
Вложения
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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
Вложения
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Robert Haas
Дата:
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
Re: [HACKERS] Tuple-routing for certain partitioned tables notworking as expected
От
Etsuro Fujita
Дата:
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