On 2016/01/14 21:36, Rushabh Lathia wrote:
On Thu, Jan 14, 2016 at 2:00 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
On 2016/01/12 20:31, Rushabh Lathia wrote:
On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp
<mailto:fujita.ets...@lab.ntt.co.jp>
<mailto:fujita.ets...@lab.ntt.co.jp
<mailto:fujita.ets...@lab.ntt.co.jp>>> wrote:
On 2016/01/06 18:58, Rushabh Lathia wrote:
.) What the need of following change ?
@@ -833,9 +833,6 @@ appendWhereClause(StringInfo buf,
int nestlevel;
ListCell *lc;
- if (params)
- *params = NIL; /* initialize result
list to
empty */
-
/* Set up context struct for recursion */
context.root = root;
context.foreignrel = baserel;
@@ -971,6 +968,63 @@ deparseUpdateSql(StringInfo buf,
PlannerInfo *root,
}
It is needed for deparsePushedDownUpdateSql to store params
in both
WHERE clauses and expressions to assign to the target columns
into one params_list list.
Hmm sorry but I am still not getting the point, can you provide some
example to explain this ?
Sorry, maybe my explanation was not enough. Consider:
postgres=# create foreign table ft1 (a int, b int) server myserver
options (table_name 't1');
postgres=# insert into ft1 values (0, 0);
postgres=# prepare mt(int, int) as update ft1 set a = $1 where b = $2;
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
After the 5 executions of mt we have
postgres=# explain verbose execute mt(1, 0);
QUERY PLAN
------------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..140.35 rows=12 width=10)
-> Foreign Update on public.ft1 (cost=100.00..140.35 rows=12
width=10)
Remote SQL: UPDATE public.t1 SET a = $1::integer WHERE ((b
= $2::integer))
(3 rows)
If we do that initialization in appendWhereClause, we would get a
wrong params_list list and a wrong remote pushed-down query for the
last mt() in deparsePushedDownUpdateSql.
Strange, I am seeing same behaviour with or without that initialization in
appendWhereClause. After the 5 executions of mt I with or without I am
getting following output:
postgres=# explain verbose execute mt(1, 0);
QUERY PLAN
------------------------------------------------------------------------------------
Update on public.ft2 (cost=100.00..140.35 rows=12 width=10)
-> Foreign Update on public.ft2 (cost=100.00..140.35 rows=12 width=10)
Remote SQL: UPDATE public.t2 SET a = $1::integer WHERE ((b =
$2::integer))
(3 rows)
Really? With that initialization in appendWhereClause, I got the
following wrong result (note that both parameter numbers are $1):
postgres=# explain verbose execute mt(1, 0);
QUERY PLAN
------------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..140.35 rows=12 width=10)
-> Foreign Update on public.ft1 (cost=100.00..140.35 rows=12 width=10)
Remote SQL: UPDATE public.t1 SET a = $1::integer WHERE ((b =
$1::integer))
(3 rows)
BTW, I keep a ForeignScan node pushing down an update to the remote
server, in the updated patches. I have to admit that that seems
like rather a misnomer. So, it might be worth adding a new
ForeignUpdate node, but my concern about that is that if doing so,
we would have a lot of duplicate code in ForeignUpdate and
ForeignScan. What do you think about that?
Yes, I noticed that in the patch and I was about to point that out in my
final review. As first review I was mainly focused on the functionality
testing
and other overview things. Another reason I haven't posted that in my
first review round is, I was not quite sure whether we need the
separate new node ForeignUpdate, ForeignDelete and want to duplicate
code? Was also not quite sure about the fact that what we will achieve
by doing that.
So I thought, I will have this open question in my final review comment,
and will take committer's opinion on this. Since you already raised this
question lets take others opinion on this.
OK, let's do that.
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
