Oddity in tuple routing for foreign partitions

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Oddity in tuple routing for foreign partitions

Etsuro Fujita
Hi,

While updating the fix-postgres_fdw-WCO-handling patch, I noticed that
the patch for tuple routing for foreign partitions doesn't work well
with remote triggers.  Here is an example:

postgres=# create table loct1 (a int check (a in (1)), b text);
postgres=# create function br_insert_trigfunc() returns trigger as
$$begin new.b := new.b || ' triggered !'; return new; end$$ language
plpgsql;
postgres=# create trigger loct1_br_insert_trigger before insert on loct1
for each row execute procedure br_insert_trigfunc();
postgres=# create table itrtest (a int, b text) partition by list (a);
postgres=# create foreign table remp1 (a int check (a in (1)), b text)
server loopback options (table_name 'loct1');
postgres=# alter table itrtest attach partition remp1 for values in (1);

This behaves oddly:

postgres=# insert into itrtest values (1, 'foo') returning *;
 a |  b
---+-----
 1 | foo
(1 row)

INSERT 0 1

The new value of b in the RETURNING result should be concatenated with '
triggered !', as shown below:

postgres=# select tableoid::regclass, * from itrtest ;
 tableoid | a |        b
----------+---+-----------------
 remp1    | 1 | foo triggered !
(1 row)

The reason for that is: since that in ExecInitPartitionInfo, the core
calls BeginForeignInsert via ExecInitRoutingInfo before initializing
ri_returningList of ResultRelInfo for the partition, BeginForeignInsert
sees that the ri_returningList is NULL and incorrectly recognizes that
the local query doesn't have a RETURNING list.  So, I moved the
ExecInitRoutingInfo call after building the ri_returningList (and before
handling ON CONFLICT because we reference the conversion map created by
that when handling that clause).  The first version of the patch called
BeginForeignInsert that order, but I updated the patch incorrectly.  :(
 Also, I removed the CheckValidResultRel check from ExecInitRoutingInfo
and added that to ExecInitPartitionInfo, right after the
InitResultRelInfo call, because it would be better to abort the
operation as soon as we find the partition to be non-routable.  Another
thing I noticed is the RT index of the foreign partition set to 1 in
postgresBeginForeignInsert to create a remote query; that would not work
for cases where the partitioned table has an RT index larger than 1 (eg,
the partitioned table is not the primary ModifyTable node); in which
cases, since the varno of Vars belonging to the foreign partition in the
RETURNING expressions is the same as the partitioned table, calling
deparseReturningList with the RT index set to 1 against the RETURNING
expressions would produce attrs_used to be NULL, leading to postgres_fdw
not retrieving actually inserted data from the remote, even if remote
triggers might change those data.  So, I fixed this as well, by setting
the RT index accordingly to the partitioned table.  Attached is a patch
for fixing these issues.  I'll add this to the open items list for PG11.
 (After addressing this, I'll post an updated version of the
fix-postgres_fdw-WCO-handling patch.)

Best regards,
Etsuro Fujita

fix-tuple-routing-for-foreign-partitions.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Amit Langote-2
Fujita-san,

On 2018/04/16 20:25, Etsuro Fujita wrote:

> Hi,
>
> While updating the fix-postgres_fdw-WCO-handling patch, I noticed that
> the patch for tuple routing for foreign partitions doesn't work well
> with remote triggers.  Here is an example:
>
> postgres=# create table loct1 (a int check (a in (1)), b text);
> postgres=# create function br_insert_trigfunc() returns trigger as
> $$begin new.b := new.b || ' triggered !'; return new; end$$ language
> plpgsql;
> postgres=# create trigger loct1_br_insert_trigger before insert on loct1
> for each row execute procedure br_insert_trigfunc();
> postgres=# create table itrtest (a int, b text) partition by list (a);
> postgres=# create foreign table remp1 (a int check (a in (1)), b text)
> server loopback options (table_name 'loct1');
> postgres=# alter table itrtest attach partition remp1 for values in (1);
>
> This behaves oddly:
>
> postgres=# insert into itrtest values (1, 'foo') returning *;
>  a |  b
> ---+-----
>  1 | foo
> (1 row)
>
> INSERT 0 1
>
> The new value of b in the RETURNING result should be concatenated with '
> triggered !', as shown below:
>
> postgres=# select tableoid::regclass, * from itrtest ;
>  tableoid | a |        b
> ----------+---+-----------------
>  remp1    | 1 | foo triggered !
> (1 row)
>
> The reason for that is: since that in ExecInitPartitionInfo, the core
> calls BeginForeignInsert via ExecInitRoutingInfo before initializing
> ri_returningList of ResultRelInfo for the partition, BeginForeignInsert
> sees that the ri_returningList is NULL and incorrectly recognizes that
> the local query doesn't have a RETURNING list.

Good catch!

> So, I moved the
> ExecInitRoutingInfo call after building the ri_returningList (and before
> handling ON CONFLICT because we reference the conversion map created by
> that when handling that clause).  The first version of the patch called
> BeginForeignInsert that order, but I updated the patch incorrectly.  :(

I didn't notice that when reviewing either.  Actually, I was under the
impression that BeginForeignInsert consumes returningList from the
ModifyTable node itself, not the ResultRelInfo, but now I see that
ri_returningList was added exactly for BeginForeignInsert to consume.
Good thing about that is that we get a Var-translated version instead of
the original one that contains parent's attnos.

> Also, I removed the CheckValidResultRel check from ExecInitRoutingInfo
> and added that to ExecInitPartitionInfo right after the> InitResultRelInfo call, because it would be better to abort the
> operation as soon as we find the partition to be non-routable.

Sounds fine.

> Another
> thing I noticed is the RT index of the foreign partition set to 1 in
> postgresBeginForeignInsert to create a remote query; that would not work
> for cases where the partitioned table has an RT index larger than 1 (eg,
> the partitioned table is not the primary ModifyTable node); in which
> cases, since the varno of Vars belonging to the foreign partition in the
> RETURNING expressions is the same as the partitioned table, calling
> deparseReturningList with the RT index set to 1 against the RETURNING
> expressions would produce attrs_used to be NULL, leading to postgres_fdw
> not retrieving actually inserted data from the remote, even if remote
> triggers might change those data.  So, I fixed this as well, by setting
> the RT index accordingly to the partitioned table.

Check.

> Attached is a patch
> for fixing these issues.  I'll add this to the open items list for PG11.
>  (After addressing this, I'll post an updated version of the
> fix-postgres_fdw-WCO-handling patch.)

Your patch seems to fix the issue and code changes seem fine to me.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Etsuro Fujita
Hi Amit,

(2018/04/17 10:10), Amit Langote wrote:

> On 2018/04/16 20:25, Etsuro Fujita wrote:
>> While updating the fix-postgres_fdw-WCO-handling patch, I noticed that
>> the patch for tuple routing for foreign partitions doesn't work well
>> with remote triggers.  Here is an example:
>>
>> postgres=# create table loct1 (a int check (a in (1)), b text);
>> postgres=# create function br_insert_trigfunc() returns trigger as
>> $$begin new.b := new.b || ' triggered !'; return new; end$$ language
>> plpgsql;
>> postgres=# create trigger loct1_br_insert_trigger before insert on loct1
>> for each row execute procedure br_insert_trigfunc();
>> postgres=# create table itrtest (a int, b text) partition by list (a);
>> postgres=# create foreign table remp1 (a int check (a in (1)), b text)
>> server loopback options (table_name 'loct1');
>> postgres=# alter table itrtest attach partition remp1 for values in (1);
>>
>> This behaves oddly:
>>
>> postgres=# insert into itrtest values (1, 'foo') returning *;
>>   a |  b
>> ---+-----
>>   1 | foo
>> (1 row)
>>
>> INSERT 0 1
>>
>> The new value of b in the RETURNING result should be concatenated with '
>> triggered !', as shown below:
>>
>> postgres=# select tableoid::regclass, * from itrtest ;
>>   tableoid | a |        b
>> ----------+---+-----------------
>>   remp1    | 1 | foo triggered !
>> (1 row)
>>
>> The reason for that is: since that in ExecInitPartitionInfo, the core
>> calls BeginForeignInsert via ExecInitRoutingInfo before initializing
>> ri_returningList of ResultRelInfo for the partition, BeginForeignInsert
>> sees that the ri_returningList is NULL and incorrectly recognizes that
>> the local query doesn't have a RETURNING list.
>
> Good catch!
>
>> So, I moved the
>> ExecInitRoutingInfo call after building the ri_returningList (and before
>> handling ON CONFLICT because we reference the conversion map created by
>> that when handling that clause).  The first version of the patch called
>> BeginForeignInsert that order, but I updated the patch incorrectly.  :(
>
> I didn't notice that when reviewing either.  Actually, I was under the
> impression that BeginForeignInsert consumes returningList from the
> ModifyTable node itself, not the ResultRelInfo, but now I see that
> ri_returningList was added exactly for BeginForeignInsert to consume.
> Good thing about that is that we get a Var-translated version instead of
> the original one that contains parent's attnos.
>
>> Also, I removed the CheckValidResultRel check from ExecInitRoutingInfo
>> and added that to ExecInitPartitionInfo right after the>  InitResultRelInfo call, because it would be better to abort the
>> operation as soon as we find the partition to be non-routable.
>
> Sounds fine.
>
>> Another
>> thing I noticed is the RT index of the foreign partition set to 1 in
>> postgresBeginForeignInsert to create a remote query; that would not work
>> for cases where the partitioned table has an RT index larger than 1 (eg,
>> the partitioned table is not the primary ModifyTable node); in which
>> cases, since the varno of Vars belonging to the foreign partition in the
>> RETURNING expressions is the same as the partitioned table, calling
>> deparseReturningList with the RT index set to 1 against the RETURNING
>> expressions would produce attrs_used to be NULL, leading to postgres_fdw
>> not retrieving actually inserted data from the remote, even if remote
>> triggers might change those data.  So, I fixed this as well, by setting
>> the RT index accordingly to the partitioned table.
>
> Check.
>
>> Attached is a patch
>> for fixing these issues.  I'll add this to the open items list for PG11.
>>   (After addressing this, I'll post an updated version of the
>> fix-postgres_fdw-WCO-handling patch.)
>
> Your patch seems to fix the issue and code changes seem fine to me.

Thanks for the review!

Best regards,
Etsuro Fujita

Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Kyotaro HORIGUCHI-2
In reply to this post by Amit Langote-2
Hello.

At Tue, 17 Apr 2018 10:10:38 +0900, Amit Langote <[hidden email]> wrote in <[hidden email]>

> Fujita-san,
>
> On 2018/04/16 20:25, Etsuro Fujita wrote:
> > Hi,
> >
> > While updating the fix-postgres_fdw-WCO-handling patch, I noticed that
> > the patch for tuple routing for foreign partitions doesn't work well
> > with remote triggers.  Here is an example:
> >
> > postgres=# create table loct1 (a int check (a in (1)), b text);
> > postgres=# create function br_insert_trigfunc() returns trigger as
> > $$begin new.b := new.b || ' triggered !'; return new; end$$ language
> > plpgsql;
> > postgres=# create trigger loct1_br_insert_trigger before insert on loct1
> > for each row execute procedure br_insert_trigfunc();
> > postgres=# create table itrtest (a int, b text) partition by list (a);
> > postgres=# create foreign table remp1 (a int check (a in (1)), b text)
> > server loopback options (table_name 'loct1');
> > postgres=# alter table itrtest attach partition remp1 for values in (1);
> >
> > This behaves oddly:
> >
> > postgres=# insert into itrtest values (1, 'foo') returning *;
> >  a |  b
> > ---+-----
> >  1 | foo
> > (1 row)
> >
> > INSERT 0 1
> >
> > The new value of b in the RETURNING result should be concatenated with '
> > triggered !', as shown below:
> >
> > postgres=# select tableoid::regclass, * from itrtest ;
> >  tableoid | a |        b
> > ----------+---+-----------------
> >  remp1    | 1 | foo triggered !
> > (1 row)
> >
> > The reason for that is: since that in ExecInitPartitionInfo, the core
> > calls BeginForeignInsert via ExecInitRoutingInfo before initializing
> > ri_returningList of ResultRelInfo for the partition, BeginForeignInsert
> > sees that the ri_returningList is NULL and incorrectly recognizes that
> > the local query doesn't have a RETURNING list.
>
> Good catch!
>
> > So, I moved the
> > ExecInitRoutingInfo call after building the ri_returningList (and before
> > handling ON CONFLICT because we reference the conversion map created by
> > that when handling that clause).  The first version of the patch called
> > BeginForeignInsert that order, but I updated the patch incorrectly.  :(
>
> I didn't notice that when reviewing either.  Actually, I was under the
> impression that BeginForeignInsert consumes returningList from the
> ModifyTable node itself, not the ResultRelInfo, but now I see that
> ri_returningList was added exactly for BeginForeignInsert to consume.
> Good thing about that is that we get a Var-translated version instead of
> the original one that contains parent's attnos.
>
> > Also, I removed the CheckValidResultRel check from ExecInitRoutingInfo
> > and added that to ExecInitPartitionInfo right after the> InitResultRelInfo call, because it would be better to abort the
> > operation as soon as we find the partition to be non-routable.
>
> Sounds fine.

If I'm reading this correctly, ExecInitParititionInfo calls
ExecInitRoutingInfo so currently CheckValidityResultRel is called
for the child when partrel is created in ExecPrepareTupleRouting.
But the move of CheckValidResultRel results in letting partrel
just created in ExecPrepareTupleRouting not be checked at all
since ri_ParititionReadyForRouting is always set true in
ExecInitPartitionInfo.

> > Another
> > thing I noticed is the RT index of the foreign partition set to 1 in
> > postgresBeginForeignInsert to create a remote query; that would not work
> > for cases where the partitioned table has an RT index larger than 1 (eg,
> > the partitioned table is not the primary ModifyTable node); in which
> > cases, since the varno of Vars belonging to the foreign partition in the
> > RETURNING expressions is the same as the partitioned table, calling
> > deparseReturningList with the RT index set to 1 against the RETURNING
> > expressions would produce attrs_used to be NULL, leading to postgres_fdw
> > not retrieving actually inserted data from the remote, even if remote
> > triggers might change those data.  So, I fixed this as well, by setting
> > the RT index accordingly to the partitioned table.
>
> Check.

I'm not sure but is it true that the parent is always at
resultRelation - 1?

> > Attached is a patch
> > for fixing these issues.  I'll add this to the open items list for PG11.
> >  (After addressing this, I'll post an updated version of the
> > fix-postgres_fdw-WCO-handling patch.)
>
> Your patch seems to fix the issue and code changes seem fine to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Amit Langote-2
On 2018/04/17 11:13, Kyotaro HORIGUCHI wrote:

>>> Also, I removed the CheckValidResultRel check from ExecInitRoutingInfo
>>> and added that to ExecInitPartitionInfo right after the> InitResultRelInfo call,
>>> because it would be better to abort the
>>> operation as soon as we find the partition to be non-routable.
>>
>> Sounds fine.
>
> If I'm reading this correctly, ExecInitParititionInfo calls
> ExecInitRoutingInfo so currently CheckValidityResultRel is called
> for the child when partrel is created in ExecPrepareTupleRouting.
> But the move of CheckValidResultRel results in letting partrel
> just created in ExecPrepareTupleRouting not be checked at all
> since ri_ParititionReadyForRouting is always set true in
> ExecInitPartitionInfo.

I thought the same upon reading the email, but it seems that the patch
does add CheckValidResultRel() in ExecInitPartitionInfo() as well:

@@ -335,6 +335,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
                       estate->es_instrument);

     /*
+     * Verify result relation is a valid target for an INSERT.  An UPDATE
of a
+     * partition-key becomes a DELETE+INSERT operation, so this check is
still
+     * required when the operation is CMD_UPDATE.
+     */
+    CheckValidResultRel(leaf_part_rri, CMD_INSERT);
+
+    /*
      * Since we've just initialized this ResultRelInfo, it's not in any list
      * attached to the estate as yet.  Add it, so that it can be found later.
      *


>>> Another
>>> thing I noticed is the RT index of the foreign partition set to 1 in
>>> postgresBeginForeignInsert to create a remote query; that would not work
>>> for cases where the partitioned table has an RT index larger than 1 (eg,
>>> the partitioned table is not the primary ModifyTable node); in which
>>> cases, since the varno of Vars belonging to the foreign partition in the
>>> RETURNING expressions is the same as the partitioned table, calling
>>> deparseReturningList with the RT index set to 1 against the RETURNING
>>> expressions would produce attrs_used to be NULL, leading to postgres_fdw
>>> not retrieving actually inserted data from the remote, even if remote
>>> triggers might change those data.  So, I fixed this as well, by setting
>>> the RT index accordingly to the partitioned table.
>>
>> Check.
>
> I'm not sure but is it true that the parent is always at
> resultRelation - 1?

Unless I'm missing something, EState.es_range_table contains *all* range
table entries that were created by the planner.  So, if the planner
assigned resultRelation as the RT index for the parent, then it seems we
can rely on getting the same entry back with
list_nth_cell(estate->es_range_table, resultRelation - 1).  That seems
true even if the parent wasn't the *primary* target relation.  For
example, the following works with the patch:

-- in addition to the tables shown in Fujita-san's email, define one more
-- local partition
create table locp2 partition of itrtest for values in (2);

-- simple case
insert into itrtest values (1, 'foo') returning *;
 a |        b
---+-----------------
 1 | foo triggered !
(1 row)

-- itrtest (the parent) appears as both the primary target relation and
-- otherwise.  The latter in the form of being mentioned in the with query

with ins (a, b) as (insert into itrtest values (1, 'foo') returning *)
insert into itrtest select a + 1, b from ins returning *;
 a |        b
---+-----------------
 2 | foo triggered !
(1 row)

But maybe I misunderstood your question.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Etsuro Fujita
(2018/04/17 16:10), Amit Langote wrote:

> On 2018/04/17 11:13, Kyotaro HORIGUCHI wrote:
>> If I'm reading this correctly, ExecInitParititionInfo calls
>> ExecInitRoutingInfo so currently CheckValidityResultRel is called
>> for the child when partrel is created in ExecPrepareTupleRouting.
>> But the move of CheckValidResultRel results in letting partrel
>> just created in ExecPrepareTupleRouting not be checked at all
>> since ri_ParititionReadyForRouting is always set true in
>> ExecInitPartitionInfo.
>
> I thought the same upon reading the email, but it seems that the patch
> does add CheckValidResultRel() in ExecInitPartitionInfo() as well:
>
> @@ -335,6 +335,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
>                         estate->es_instrument);
>
>       /*
> +     * Verify result relation is a valid target for an INSERT.  An UPDATE
> of a
> +     * partition-key becomes a DELETE+INSERT operation, so this check is
> still
> +     * required when the operation is CMD_UPDATE.
> +     */
> +    CheckValidResultRel(leaf_part_rri, CMD_INSERT);
> +
> +    /*
>        * Since we've just initialized this ResultRelInfo, it's not in any list
>        * attached to the estate as yet.  Add it, so that it can be found later.
>        *

That's right.  So, the validity check would be applied to a partition
created in ExecPrepareTupleRouting as well.

>>>> Another
>>>> thing I noticed is the RT index of the foreign partition set to 1 in
>>>> postgresBeginForeignInsert to create a remote query; that would not work
>>>> for cases where the partitioned table has an RT index larger than 1 (eg,
>>>> the partitioned table is not the primary ModifyTable node); in which
>>>> cases, since the varno of Vars belonging to the foreign partition in the
>>>> RETURNING expressions is the same as the partitioned table, calling
>>>> deparseReturningList with the RT index set to 1 against the RETURNING
>>>> expressions would produce attrs_used to be NULL, leading to postgres_fdw
>>>> not retrieving actually inserted data from the remote, even if remote
>>>> triggers might change those data.  So, I fixed this as well, by setting
>>>> the RT index accordingly to the partitioned table.
>>>
>>> Check.
>>
>> I'm not sure but is it true that the parent is always at
>> resultRelation - 1?
>
> Unless I'm missing something, EState.es_range_table contains *all* range
> table entries that were created by the planner.  So, if the planner
> assigned resultRelation as the RT index for the parent, then it seems we
> can rely on getting the same entry back with
> list_nth_cell(estate->es_range_table, resultRelation - 1).  That seems
> true even if the parent wasn't the *primary* target relation.  For
> example, the following works with the patch:
>
> -- in addition to the tables shown in Fujita-san's email, define one more
> -- local partition
> create table locp2 partition of itrtest for values in (2);
>
> -- simple case
> insert into itrtest values (1, 'foo') returning *;
>   a |        b
> ---+-----------------
>   1 | foo triggered !
> (1 row)
>
> -- itrtest (the parent) appears as both the primary target relation and
> -- otherwise.  The latter in the form of being mentioned in the with query
>
> with ins (a, b) as (insert into itrtest values (1, 'foo') returning *)
> insert into itrtest select a + 1, b from ins returning *;
>   a |        b
> ---+-----------------
>   2 | foo triggered !
> (1 row)

I agree with that.  Thanks for the explanation and the example, Amit!

Maybe my explanation (or the naming parent_rte/child_rte in the patch)
was not necessarily good, so I guess that that confused horiguchi-san.
Let me explain.  In the INSERT/COPY-tuple-routing case, as explained by
Amit, the RTE at that position in the EState's range table is the one
for the partitioned table of a given partition, so the statement would
be true.  BUT in the UPDATE-tuple-routing case, the RTE is the one for
the given partition, not for the partitioned table, so the statement
would not be true.  In the latter case, we don't need to create a child
RTE and replace the original RTE with it, but I handled both cases the
same way for simplicity.

Thanks for the comments, Horiguchi-san!

Best regards,
Etsuro Fujita

Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Amit Langote-2
On 2018/04/17 16:41, Etsuro Fujita wrote:
> In the INSERT/COPY-tuple-routing case, as explained by Amit, the
> RTE at that position in the EState's range table is the one for the
> partitioned table of a given partition, so the statement would be true. 
> BUT in the UPDATE-tuple-routing case, the RTE is the one for the given
> partition, not for the partitioned table, so the statement would not be
> true.  In the latter case, we don't need to create a child RTE and replace
> the original RTE with it, but I handled both cases the same way for
> simplicity.

Oh, I didn't really consider this part carefully.  That the resultRelInfo
received by BeginForeignInsert (when called by ExecInitRoutingInfo) could
be a reused UPDATE result relation.  It might be possible to justify the
parent_rte/child_rte terminology by explaining it a bit better.  I see
three cases that arise during tuple routing:

1. This is INSERT and so the resultRelation that's received in
   BeginForeignInsert has been freshly created in ExecInitPartitionInfo
   and it bears node->nominalRelation or 1 as its ri_RangeTableIndex

2. This is UPDATE and the resultRelInfo that's received in
   BeginForeignInsert has been freshly created in ExecInitPartitionInfo
   and it bears node->nominalRelation or 1 as its ri_RangeTableIndex

3. This is UPDATE and the resultRelInfo that's received in
   BeginForeignInsert is a reused one, in which case, it bears the planner
   assigned ri_RangeTableIndex

In all three cases, I think we can rely on using ri_RangeTableIndex to
fetch a valid "parent" RTE from es_range_table.

Do you think we need to clarify this in a comment?

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Etsuro Fujita
(2018/04/18 14:44), Amit Langote wrote:

> On 2018/04/17 16:41, Etsuro Fujita wrote:
>> In the INSERT/COPY-tuple-routing case, as explained by Amit, the
>> RTE at that position in the EState's range table is the one for the
>> partitioned table of a given partition, so the statement would be true.
>> BUT in the UPDATE-tuple-routing case, the RTE is the one for the given
>> partition, not for the partitioned table, so the statement would not be
>> true.  In the latter case, we don't need to create a child RTE and replace
>> the original RTE with it, but I handled both cases the same way for
>> simplicity.
>
> Oh, I didn't really consider this part carefully.  That the resultRelInfo
> received by BeginForeignInsert (when called by ExecInitRoutingInfo) could
> be a reused UPDATE result relation.  It might be possible to justify the
> parent_rte/child_rte terminology by explaining it a bit better.
Yeah, I think that terminology would be confusing, so I changed it to
old_rte/new_rte.

> I see
> three cases that arise during tuple routing:
>
> 1. This is INSERT and so the resultRelation that's received in
>     BeginForeignInsert has been freshly created in ExecInitPartitionInfo
>     and it bears node->nominalRelation or 1 as its ri_RangeTableIndex

Right.

> 2. This is UPDATE and the resultRelInfo that's received in
>     BeginForeignInsert has been freshly created in ExecInitPartitionInfo
>     and it bears node->nominalRelation or 1 as its ri_RangeTableIndex

In that case, we have a valid plan node, so it would only bear
node->nominalRelation.

> 3. This is UPDATE and the resultRelInfo that's received in
>     BeginForeignInsert is a reused one, in which case, it bears the planner
>     assigned ri_RangeTableIndex

Right.

> In all three cases, I think we can rely on using ri_RangeTableIndex to
> fetch a valid "parent" RTE from es_range_table.

I slept on this, I noticed there is another bug in case #2.  Here is an
example with the previous patch:

postgres=# create table utrtest (a int, b text) partition by list (a);
postgres=# create table loct (a int check (a in (1)), b text);
postgres=# create foreign table remp (a int check (a in (1)), b text)
server loopback options (table_name 'loct');
postgres=# create table locp (a int check (a in (2)), b text);
postgres=# alter table utrtest attach partition remp for values in (1);
postgres=# alter table utrtest attach partition locp for values in (2);
postgres=# create trigger loct_br_insert_trigger before insert on loct
for each row execute procedure br_insert_trigfunc();

where the trigger function is the one defined in an earlier email.

postgres=# insert into utrtest values (2, 'qux');
INSERT 0 1
postgres=# update utrtest set a = 1 where a = 2 returning *;
  a |  b
---+-----
  1 | qux
(1 row)

UPDATE 1

Wrong result!  The result should be concatenated with ' triggered !'.

In case #2, since we initialize expressions for the partition's
ResultRelInfo including RETURNING by translating the attnos of the
corresponding expressions in the ResultRelInfo for the first subplan
target rel, I think we should replace the RTE for the first subplan
target rel, not the RTE for the nominal rel, with the new one created
for the foreign table.  Attached is an updated version for fixing this
issue.

> Do you think we need to clarify this in a comment?

Yeah, but I updated comments about this a little bit different way in
the attached.  Does that make sense?

Thanks for the comments!

Best regards,
Etsuro Fujita

fix-tuple-routing-for-foreign-partitions-2.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Amit Langote-2
On 2018/04/18 21:55, Etsuro Fujita wrote:
> (2018/04/18 14:44), Amit Langote wrote:
>> Oh, I didn't really consider this part carefully.  That the resultRelInfo
>> received by BeginForeignInsert (when called by ExecInitRoutingInfo) could
>> be a reused UPDATE result relation.  It might be possible to justify the
>> parent_rte/child_rte terminology by explaining it a bit better.
>
>> 2. This is UPDATE and the resultRelInfo that's received in
>>     BeginForeignInsert has been freshly created in ExecInitPartitionInfo
>>     and it bears node->nominalRelation or 1 as its ri_RangeTableIndex

[ ... ]

>> In all three cases, I think we can rely on using ri_RangeTableIndex to
>> fetch a valid "parent" RTE from es_range_table.
>
> I slept on this, I noticed there is another bug in case #2.  Here is an
> example with the previous patch:
>
> postgres=# create table utrtest (a int, b text) partition by list (a);
> postgres=# create table loct (a int check (a in (1)), b text);
> postgres=# create foreign table remp (a int check (a in (1)), b text)
> server loopback options (table_name 'loct');
> postgres=# create table locp (a int check (a in (2)), b text);
> postgres=# alter table utrtest attach partition remp for values in (1);
> postgres=# alter table utrtest attach partition locp for values in (2);
> postgres=# create trigger loct_br_insert_trigger before insert on loct for
> each row execute procedure br_insert_trigfunc();
>
> where the trigger function is the one defined in an earlier email.
>
> postgres=# insert into utrtest values (2, 'qux');
> INSERT 0 1
> postgres=# update utrtest set a = 1 where a = 2 returning *;
>  a |  b
> ---+-----
>  1 | qux
> (1 row)
>
> UPDATE 1
>
> Wrong result!  The result should be concatenated with ' triggered !'.
>
> In case #2, since we initialize expressions for the partition's
> ResultRelInfo including RETURNING by translating the attnos of the
> corresponding expressions in the ResultRelInfo for the first subplan
> target rel, I think we should replace the RTE for the first subplan target
> rel, not the RTE for the nominal rel, with the new one created for the
> foreign table.

Ah, so in case #2, we use firstVarno (per ExecInitPartitionInfo terms) as
varno throughout.  So, we'd like to put the new RTE in that slot.

Would it be a good idea to explain *why* we need to replace the RTE in the
first place?  Afaics, it's for deparseColumnRef() to find the correct
relation when it uses planner_rt_fetch() to get the RTE.

> Attached is an updated version for fixing this issue.
>
>> Do you think we need to clarify this in a comment?
>
> Yeah, but I updated comments about this a little bit different way in the
> attached.  Does that make sense?

It looks generally good, although in the following:

+    /*
+     * If the foreign table is a partition, temporarily replace the parent's
+     * RTE in the range table with a new target RTE describing the foreign
+     * table for use by deparseInsertSql() and create_foreign_modify() below.
+     */

.. it could be mentioned that we don't switch either the RTE or the value
assigned to resultRelation if the RTE currently at resultRelation RT index
is the one created by the planner and refers to the same relation that
resultRelInfo does.

Beside that, I noticed that the patch has a stray white-space at the end
of the following line:

+                /* partition that isn't a subplan target rel */

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Kyotaro HORIGUCHI-2
In reply to this post by Etsuro Fujita
At Tue, 17 Apr 2018 16:41:31 +0900, Etsuro Fujita <[hidden email]> wrote in <[hidden email]>

> (2018/04/17 16:10), Amit Langote wrote:
> > On 2018/04/17 11:13, Kyotaro HORIGUCHI wrote:
> >> If I'm reading this correctly, ExecInitParititionInfo calls
> >> ExecInitRoutingInfo so currently CheckValidityResultRel is called
> >> for the child when partrel is created in ExecPrepareTupleRouting.
> >> But the move of CheckValidResultRel results in letting partrel
> >> just created in ExecPrepareTupleRouting not be checked at all
> >> since ri_ParititionReadyForRouting is always set true in
> >> ExecInitPartitionInfo.
> >
> > I thought the same upon reading the email, but it seems that the patch
> > does add CheckValidResultRel() in ExecInitPartitionInfo() as well:
> >
> > @@ -335,6 +335,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
> >                         estate->es_instrument);
> >
> >       /*
> > + * Verify result relation is a valid target for an INSERT.  An UPDATE
> > of a
> > + * partition-key becomes a DELETE+INSERT operation, so this check is
> > still
> > +     * required when the operation is CMD_UPDATE.
> > +     */
> > +    CheckValidResultRel(leaf_part_rri, CMD_INSERT);
> > +
> > +    /*
> >        * Since we've just initialized this ResultRelInfo, it's not in any
> >        * list
> >        * attached to the estate as yet.  Add it, so that it can be found
> >        * later.
> >        *
>
> That's right.  So, the validity check would be applied to a partition
> created in ExecPrepareTupleRouting as well.

Yes, that's actually true but it seems quite bug-prone or at
least hard to understand. I understand that the
CheckValidResultRel(INSERT) is just a prerequisite for
ExecInitRoutingInfo but the relationship is obfucated after this
patch. If we have a strong reason to error-out as fast as
possible, the original code seems better to me..

> >>>> Another
> >>>> thing I noticed is the RT index of the foreign partition set to 1 in
> >>>> postgresBeginForeignInsert to create a remote query; that would not
> >>>> work
> >>>> for cases where the partitioned table has an RT index larger than 1
> >>>> (eg,
> >>>> the partitioned table is not the primary ModifyTable node); in which
> >>>> cases, since the varno of Vars belonging to the foreign partition in
> >>>> the
> >>>> RETURNING expressions is the same as the partitioned table, calling
> >>>> deparseReturningList with the RT index set to 1 against the RETURNING
> >>>> expressions would produce attrs_used to be NULL, leading to
> >>>> postgres_fdw
> >>>> not retrieving actually inserted data from the remote, even if remote
> >>>> triggers might change those data.  So, I fixed this as well, by
> >>>> setting
> >>>> the RT index accordingly to the partitioned table.
> >>>
> >>> Check.
> >>
> >> I'm not sure but is it true that the parent is always at
> >> resultRelation - 1?
> >
> > Unless I'm missing something, EState.es_range_table contains *all*
> > range
> > table entries that were created by the planner.  So, if the planner
> > assigned resultRelation as the RT index for the parent, then it seems
> > we
> > can rely on getting the same entry back with
> > list_nth_cell(estate->es_range_table, resultRelation - 1).  That seems
> > true even if the parent wasn't the *primary* target relation.  For
> > example, the following works with the patch:
> >
> > -- in addition to the tables shown in Fujita-san's email, define one more
> > -- local partition
> > create table locp2 partition of itrtest for values in (2);
> >
> > -- simple case
> > insert into itrtest values (1, 'foo') returning *;
> >   a |        b
> > ---+-----------------
> >   1 | foo triggered !
> > (1 row)
> >
> > -- itrtest (the parent) appears as both the primary target relation and
> > -- otherwise.  The latter in the form of being mentioned in the with
> > -- query
> >
> > with ins (a, b) as (insert into itrtest values (1, 'foo') returning *)
> > insert into itrtest select a + 1, b from ins returning *;
> >   a |        b
> > ---+-----------------
> >   2 | foo triggered !
> > (1 row)
>
> I agree with that.  Thanks for the explanation and the example, Amit!
>
> Maybe my explanation (or the naming parent_rte/child_rte in the patch)
> was not necessarily good, so I guess that that confused
> horiguchi-san. Let me explain.  In the INSERT/COPY-tuple-routing case,
> as explained by Amit, the RTE at that position in the EState's range
> table is the one for the partitioned table of a given partition, so
> the statement would be true.  BUT in the UPDATE-tuple-routing case,
> the RTE is the one for the given partition, not for the partitioned
> table, so the statement would not be true.  In the latter case, we
> don't need to create a child RTE and replace the original RTE with it,
> but I handled both cases the same way for simplicity.

Thank you, your understanding of my concern is right. Thanks for
the explanation.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Etsuro Fujita
In reply to this post by Amit Langote-2
(2018/04/19 16:43), Amit Langote wrote:
> On 2018/04/18 21:55, Etsuro Fujita wrote:
>> (2018/04/18 14:44), Amit Langote wrote:
>>> That the resultRelInfo
>>> received by BeginForeignInsert (when called by ExecInitRoutingInfo) could
>>> be a reused UPDATE result relation.

>>> 2. This is UPDATE and the resultRelInfo that's received in
>>>      BeginForeignInsert has been freshly created in ExecInitPartitionInfo
>>>      and it bears node->nominalRelation or 1 as its ri_RangeTableIndex

>>> In all three cases, I think we can rely on using ri_RangeTableIndex to
>>> fetch a valid "parent" RTE from es_range_table.
>>
>> I slept on this, I noticed there is another bug in case #2.

>> In case #2, since we initialize expressions for the partition's
>> ResultRelInfo including RETURNING by translating the attnos of the
>> corresponding expressions in the ResultRelInfo for the first subplan
>> target rel, I think we should replace the RTE for the first subplan target
>> rel, not the RTE for the nominal rel, with the new one created for the
>> foreign table.
>
> Ah, so in case #2, we use firstVarno (per ExecInitPartitionInfo terms) as
> varno throughout.  So, we'd like to put the new RTE in that slot.
>
> Would it be a good idea to explain *why* we need to replace the RTE in the
> first place?  Afaics, it's for deparseColumnRef() to find the correct
> relation when it uses planner_rt_fetch() to get the RTE.
That might be a good idea, but one thing I'm concerned about is that
since we might use the RTE in different ways in future developments,
such a comment might be obsolete rather sooner.  So, I just added *for
use by deparseInsertSql() and create_foreign_modify() below* to the
comments shown below.  But I'd like to leave this to the committer.

>> Attached is an updated version for fixing this issue.
>>
>>> Do you think we need to clarify this in a comment?
>>
>> Yeah, but I updated comments about this a little bit different way in the
>> attached.  Does that make sense?
>
> It looks generally good, although in the following:
>
> +    /*
> +     * If the foreign table is a partition, temporarily replace the parent's
> +     * RTE in the range table with a new target RTE describing the foreign
> +     * table for use by deparseInsertSql() and create_foreign_modify() below.
> +     */
>
> .. it could be mentioned that we don't switch either the RTE or the value
> assigned to resultRelation if the RTE currently at resultRelation RT index
> is the one created by the planner and refers to the same relation that
> resultRelInfo does.
Done.

> Beside that, I noticed that the patch has a stray white-space at the end
> of the following line:
>
> +                /* partition that isn't a subplan target rel */

Fixed.

Thanks for the review!  Attached is a new version of the patch.

Best regards,
Etsuro Fujita

fix-tuple-routing-for-foreign-partitions-3.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Etsuro Fujita
In reply to this post by Kyotaro HORIGUCHI-2
(2018/04/19 19:03), Kyotaro HORIGUCHI wrote:

> At Tue, 17 Apr 2018 16:41:31 +0900, Etsuro Fujita<[hidden email]>  wrote in<[hidden email]>
>> (2018/04/17 16:10), Amit Langote wrote:
>>> On 2018/04/17 11:13, Kyotaro HORIGUCHI wrote:
>>>> If I'm reading this correctly, ExecInitParititionInfo calls
>>>> ExecInitRoutingInfo so currently CheckValidityResultRel is called
>>>> for the child when partrel is created in ExecPrepareTupleRouting.
>>>> But the move of CheckValidResultRel results in letting partrel
>>>> just created in ExecPrepareTupleRouting not be checked at all
>>>> since ri_ParititionReadyForRouting is always set true in
>>>> ExecInitPartitionInfo.
>>>
>>> I thought the same upon reading the email, but it seems that the patch
>>> does add CheckValidResultRel() in ExecInitPartitionInfo() as well:
>>>
>>> @@ -335,6 +335,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
>>>                          estate->es_instrument);
>>>
>>>        /*
>>> + * Verify result relation is a valid target for an INSERT.  An UPDATE
>>> of a
>>> + * partition-key becomes a DELETE+INSERT operation, so this check is
>>> still
>>> +     * required when the operation is CMD_UPDATE.
>>> +     */
>>> +    CheckValidResultRel(leaf_part_rri, CMD_INSERT);
>>> +
>>> +    /*
>>>         * Since we've just initialized this ResultRelInfo, it's not in any
>>>         * list
>>>         * attached to the estate as yet.  Add it, so that it can be found
>>>         * later.
>>>         *
>>
>> That's right.  So, the validity check would be applied to a partition
>> created in ExecPrepareTupleRouting as well.
>
> Yes, that's actually true but it seems quite bug-prone or at
> least hard to understand. I understand that the
> CheckValidResultRel(INSERT) is just a prerequisite for
> ExecInitRoutingInfo but the relationship is obfucated after this
> patch. If we have a strong reason to error-out as fast as
> possible, the original code seems better to me..

Actually, I think that change would make the initialization for a
partition more consistent with that for a main target rel in
ExecInitModifyTable, where we first perform the CheckValidResultRel
check against a target rel, and if valid, then open indices, initializes
the FDW, initialize expressions such as WITH CHECK OPTION and RETURNING,
and so on.  That's reasonable, and I like consistency, so I'd vote for
that change.

Thanks for the review!

Best regards,
Etsuro Fujita

Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Amit Langote-2
In reply to this post by Etsuro Fujita
Fujita-san,

Thanks for the updated patch.

On 2018/04/19 21:42, Etsuro Fujita wrote:

> (2018/04/19 16:43), Amit Langote wrote:
>> Would it be a good idea to explain *why* we need to replace the RTE in the
>> first place?  Afaics, it's for deparseColumnRef() to find the correct
>> relation when it uses planner_rt_fetch() to get the RTE.
>
> That might be a good idea, but one thing I'm concerned about is that since
> we might use the RTE in different ways in future developments, such a
> comment might be obsolete rather sooner.  So, I just added *for use by
> deparseInsertSql() and create_foreign_modify() below* to the comments
> shown below.  But I'd like to leave this to the committer.

OK, fine by me.

>> It looks generally good, although in the following:
>>
>> +    /*
>> +     * If the foreign table is a partition, temporarily replace the
>> parent's
>> +     * RTE in the range table with a new target RTE describing the foreign
>> +     * table for use by deparseInsertSql() and create_foreign_modify()
>> below.
>> +     */
>>
>> .. it could be mentioned that we don't switch either the RTE or the value
>> assigned to resultRelation if the RTE currently at resultRelation RT index
>> is the one created by the planner and refers to the same relation that
>> resultRelInfo does.
>
> Done.
>
>> Beside that, I noticed that the patch has a stray white-space at the end
>> of the following line:
>>
>> +                /* partition that isn't a subplan target rel */
>
> Fixed.
>
> Thanks for the review!  Attached is a new version of the patch.

Looks good.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Etsuro Fujita
(2018/04/20 9:48), Amit Langote wrote:

> On 2018/04/19 21:42, Etsuro Fujita wrote:
>> (2018/04/19 16:43), Amit Langote wrote:
>>> Would it be a good idea to explain *why* we need to replace the RTE in the
>>> first place?  Afaics, it's for deparseColumnRef() to find the correct
>>> relation when it uses planner_rt_fetch() to get the RTE.
>>
>> That might be a good idea, but one thing I'm concerned about is that since
>> we might use the RTE in different ways in future developments, such a
>> comment might be obsolete rather sooner.  So, I just added *for use by
>> deparseInsertSql() and create_foreign_modify() below* to the comments
>> shown below.  But I'd like to leave this to the committer.
>
> OK, fine by me.
>
>>> It looks generally good, although in the following:
>>>
>>> +    /*
>>> +     * If the foreign table is a partition, temporarily replace the
>>> parent's
>>> +     * RTE in the range table with a new target RTE describing the foreign
>>> +     * table for use by deparseInsertSql() and create_foreign_modify()
>>> below.
>>> +     */
>>>
>>> .. it could be mentioned that we don't switch either the RTE or the value
>>> assigned to resultRelation if the RTE currently at resultRelation RT index
>>> is the one created by the planner and refers to the same relation that
>>> resultRelInfo does.
>>
>> Done.
>>
>>> Beside that, I noticed that the patch has a stray white-space at the end
>>> of the following line:
>>>
>>> +                /* partition that isn't a subplan target rel */
>>
>> Fixed.
>>
>> Thanks for the review!  Attached is a new version of the patch.
>
> Looks good.

Thank you!

Best regards,
Etsuro Fujita

Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Álvaro Herrera
In reply to this post by Etsuro Fujita
Robert, I think this is your turf, per 3d956d9562aa.  Are you looking
into it?

Thanks,

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

Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Robert Haas
On Mon, Apr 23, 2018 at 11:25 AM, Alvaro Herrera
<[hidden email]> wrote:
> Robert, I think this is your turf, per 3d956d9562aa.  Are you looking
> into it?

Thanks for the ping.  I just had a look over the proposed patch and I
guess I don't like it very much.  Temporarily modifying the range
table in place and then changing it back before we return seems ugly
and error-prone.  I hope we can come up with a solution that doesn't
involve needing to do that.

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

Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Robert Haas
On Tue, Apr 24, 2018 at 12:21 PM, Robert Haas <[hidden email]> wrote:

> On Mon, Apr 23, 2018 at 11:25 AM, Alvaro Herrera
> <[hidden email]> wrote:
>> Robert, I think this is your turf, per 3d956d9562aa.  Are you looking
>> into it?
>
> Thanks for the ping.  I just had a look over the proposed patch and I
> guess I don't like it very much.  Temporarily modifying the range
> table in place and then changing it back before we return seems ugly
> and error-prone.  I hope we can come up with a solution that doesn't
> involve needing to do that.
I have done some refactoring to avoid that.  See attached.

I didn't really get beyond the refactoring stage with this today.
This version still seems to work, but I don't really understand the
logic in postgresBeginForeignInsert which decides whether to use the
RTE from the range table or create our own.  We seem to need to do one
sometimes and the other sometimes, but I don't know why that is,
really.  Nor do I understand why we need the other changes in the
patch.  There's probably a good reason, but I haven't figured it out
yet.

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

fix-tuple-routing-for-foreign-partitions-4.patch (34K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Kyotaro HORIGUCHI-2
Hello.

At Tue, 24 Apr 2018 15:49:20 -0400, Robert Haas <[hidden email]> wrote in <[hidden email]>

> On Tue, Apr 24, 2018 at 12:21 PM, Robert Haas <[hidden email]> wrote:
> > On Mon, Apr 23, 2018 at 11:25 AM, Alvaro Herrera
> > <[hidden email]> wrote:
> >> Robert, I think this is your turf, per 3d956d9562aa.  Are you looking
> >> into it?
> >
> > Thanks for the ping.  I just had a look over the proposed patch and I
> > guess I don't like it very much.  Temporarily modifying the range
> > table in place and then changing it back before we return seems ugly
> > and error-prone.  I hope we can come up with a solution that doesn't
> > involve needing to do that.
>
> I have done some refactoring to avoid that.  See attached.
>
> I didn't really get beyond the refactoring stage with this today.
> This version still seems to work, but I don't really understand the
> logic in postgresBeginForeignInsert which decides whether to use the
> RTE from the range table or create our own.  We seem to need to do one
> sometimes and the other sometimes, but I don't know why that is,
> really.  Nor do I understand why we need the other changes in the
> patch.  There's probably a good reason, but I haven't figured it out
> yet.

FWIW, the refactoring drops the condition that the ForeignInsert
is a part of UPDATE.  The code exists just for avoiding temprary
RTE generation in 2 cases out of the 3 possible cases mentioned
in [1]. If it looks too complex for the gain, we can always
create an RTE for deparsing as Fujita-san's first patch in this
thread did. Anyway the condition for "dostuff" + "is update"
might be a bit too arbitrary.

[1] https://www.postgresql.org/message-id/f970d875-9711-b8cb-f270-965fa3e40334@...

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Amit Langote-2
In reply to this post by Robert Haas
On 2018/04/25 4:49, Robert Haas wrote:

> On Tue, Apr 24, 2018 at 12:21 PM, Robert Haas <[hidden email]> wrote:
>> On Mon, Apr 23, 2018 at 11:25 AM, Alvaro Herrera
>> <[hidden email]> wrote:
>>> Robert, I think this is your turf, per 3d956d9562aa.  Are you looking
>>> into it?
>>
>> Thanks for the ping.  I just had a look over the proposed patch and I
>> guess I don't like it very much.  Temporarily modifying the range
>> table in place and then changing it back before we return seems ugly
>> and error-prone.  I hope we can come up with a solution that doesn't
>> involve needing to do that.
>
> I have done some refactoring to avoid that.  See attached.

+1 for getting rid of the PlannerInfo argument of the many functions in
that code.  I have long wondered if we couldn't rid of it and especially
thought of it when reviewing this patch.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Oddity in tuple routing for foreign partitions

Kyotaro HORIGUCHI-2
I forgot to mention that.

At Wed, 25 Apr 2018 10:17:02 +0900, Amit Langote <[hidden email]> wrote in <[hidden email]>

> On 2018/04/25 4:49, Robert Haas wrote:
> > On Tue, Apr 24, 2018 at 12:21 PM, Robert Haas <[hidden email]> wrote:
> >> On Mon, Apr 23, 2018 at 11:25 AM, Alvaro Herrera
> >> <[hidden email]> wrote:
> >>> Robert, I think this is your turf, per 3d956d9562aa.  Are you looking
> >>> into it?
> >>
> >> Thanks for the ping.  I just had a look over the proposed patch and I
> >> guess I don't like it very much.  Temporarily modifying the range
> >> table in place and then changing it back before we return seems ugly
> >> and error-prone.  I hope we can come up with a solution that doesn't
> >> involve needing to do that.
> >
> > I have done some refactoring to avoid that.  See attached.
>
> +1 for getting rid of the PlannerInfo argument of the many functions in
> that code.  I have long wondered if we couldn't rid of it and especially
> thought of it when reviewing this patch.

+1 from me. Thanks for making things simpler and easy to
understand. I feel the same as Amit:p

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


123