Re: [HACKERS] Postgres_fdw behaves oddly

[Yugo Nagata]

Hi,

On Fri, 3 Feb 2017 18:12:01 +0900
vinayak  wrote:

> Hello,
> 
> I have tested some scenarios of inserting data into two foreign tables 
> using postgres_fdw. All the test cases works fine except Test 5.
> 
> In Test 5, I am expecting error as both the rows violates the 
> constraint. But at the COMMIT time transaction does not give any error 
> and it takes lock waiting for a transaction to finish.

I can reproduce this with REL9_6_STABLE.

The local process (application_name = psql) is waiting 
"COMMIT TRANSACTION" for returning at pgfdw_xact_callback()
 (in postgres_fdw/connection.c), and the remote process
(application_name = postgres_fdw) is stuck at _bt_doinsert()
with XactLockTableWait.

I attached the backtrace results.

I can't figure out yet why _bt_check_unique() returns without
calling ereport().

Regards,

> 
> Please check the below tests:
> 
> postgres=# CREATE SERVER loopback1 FOREIGN DATA WRAPPER POSTGRES_FDW 
> OPTIONS (dbname 'postgres');
> CREATE SERVER
> postgres=# CREATE SERVER loopback2 FOREIGN DATA WRAPPER POSTGRES_FDW 
> OPTIONS (dbname 'postgres');
> CREATE SERVER
> postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER loopback1;
> CREATE USER MAPPING
> postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER loopback2;
> CREATE USER MAPPING
> 
> -- Here local table is created to refer as foreign table. The table has 
> constraints which are deferred till end of transaction.
> -- This allows COMMIT time errors to occur by inserting data which 
> violates constraints.
> 
> postgres=# *CREATE TABLE lt(val int UNIQUE DEFERRABLE INITIALLY DEFERRED);*
> CREATE TABLE
> postgres=# CREATE FOREIGN TABLE ft1_lt (val int) SERVER loopback1 
> OPTIONS (table_name 'lt');
> CREATE FOREIGN TABLE
> postgres=# CREATE FOREIGN TABLE ft2_lt (val int) SERVER loopback2 
> OPTIONS (table_name 'lt');
> CREATE FOREIGN TABLE
> 
> *Test 1: **
> **===*
> In a transaction insert two rows one each to the two foreign tables and 
> it works fine.
> 
> postgres=# BEGIN;
> BEGIN
> postgres=# INSERT INTO ft1_lt VALUES (1);
> INSERT 0 1
> postgres=# INSERT INTO ft2_lt VALUES (3);
> INSERT 0 1
> postgres=# COMMIT;
> COMMIT
> postgres=# SELECT * FROM lt;
>   val
> -
> 1
> 3
> (2 rows)
> 
> *Test 2:**
> **===*
> In a transaction insert two rows one each to the two foreign tables.
> One of the rows violates the constraint and other not. At the time of 
> COMMIT one of the foreign server violates the constraints so it return 
> error. I think this is also expected behavior.
> postgres=# BEGIN;
> BEGIN
> postgres=# INSERT INTO ft1_lt VALUES (1); -- Violates constraint
> INSERT 0 1
> postgres=# INSERT INTO ft2_lt VALUES (4);
> INSERT 0 1
> postgres=# COMMIT;
> 2017-02-03 15:26:28.667 JST [3081] ERROR:  duplicate key value violates 
> unique constraint "lt_val_key"
> 2017-02-03 15:26:28.667 JST [3081] DETAIL:  Key (val)=(1) already exists.
> 2017-02-03 15:26:28.667 JST [3081] STATEMENT:  COMMIT TRANSACTION
> 2017-02-03 15:26:28.668 JST [3075] ERROR:  duplicate key value violates 
> unique constraint "lt_val_key"
> 2017-02-03 15:26:28.668 JST [3075] DETAIL:  Key (val)=(1) already exists.
> 2017-02-03 15:26:28.668 JST [3075] CONTEXT:  Remote SQL command: COMMIT 
> TRANSACTION
> 2017-02-03 15:26:28.668 JST [3075] STATEMENT:  COMMIT;
> 2017-02-03 15:26:28.668 JST [3081] WARNING:  there is no transaction in 
> progress
> WARNING:  there is no transaction in progress
> ERROR:  duplicate key value violates unique constraint "lt_val_key"
> DETAIL:  Key (val)=(1) already exists.
> CONTEXT:  Remote SQL command: COMMIT TRANSACTION
> postgres=#
> postgres=#
> postgres=# SELECT * FROM lt;
>   val
> -
> 1
> 3
> (2 rows)
> 
> *Test 3:**
> **===*
> In a transaction insert two rows one each to the two foreign tables.
> One of the rows violates the constraint and other not. At the time of 
> COMMIT one of the foreign server violates the constraints so it return 
> error. I think this is also expected behavior.
> postgres=# BEGIN;
> BEGIN
> postgres=# INSERT INTO ft1_lt VALUES (4);
> INSERT 0 1
> postgres=# INSERT INTO ft2_lt VALUES (3); -- Violates constraint
> INSERT 0 1
> postgres=# COMMIT;
> 2017-02-03 15:27:14.331 JST [3084] ERROR:  duplicate key value violates 
> unique constraint "lt_val_key"
> 2017-02-03 15:27:14.331 JST [3084] DETAIL:  Key (val)=(3) already exists.
> 2017-02-03 15:27:14.331 JST [3084] STATEMENT:  COMMIT TRANSACTION
> 2017-02-03 15:27:14.332 JST [3075] ERROR:  duplicate key value violates 
> unique constraint "lt_val_key"
> 2017-02-03 15:27:14.332 JST [3075] DETAIL:  Key (val)=(3) already exists.
> 2017-02-03 15:27:14.332 JST [3075] CONTEXT:  Remote SQL command: COMMIT 
> TRANSACTION
> 2017-02-03 15:27:14.332 JST [3075] STATEMENT:  COMMIT;
> 2017-02-03 15:27:14.332 JST [3084] WARNING:  there is no transaction in 
> progress
> WARNING:  there is no transaction in progress
> ERROR:  duplicate key value violates unique constraint "lt_val_key"
> 

[HACKERS] Postgres_fdw behaves oddly

[vinayak]

Hello,

I have tested some scenarios of inserting data into two foreign tables 
using postgres_fdw. All the test cases works fine except Test 5.


In Test 5, I am expecting error as both the rows violates the 
constraint. But at the COMMIT time transaction does not give any error 
and it takes lock waiting for a transaction to finish.


Please check the below tests:

postgres=# CREATE SERVER loopback1 FOREIGN DATA WRAPPER POSTGRES_FDW 
OPTIONS (dbname 'postgres');

CREATE SERVER
postgres=# CREATE SERVER loopback2 FOREIGN DATA WRAPPER POSTGRES_FDW 
OPTIONS (dbname 'postgres');

CREATE SERVER
postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER loopback1;
CREATE USER MAPPING
postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER loopback2;
CREATE USER MAPPING

-- Here local table is created to refer as foreign table. The table has 
constraints which are deferred till end of transaction.
-- This allows COMMIT time errors to occur by inserting data which 
violates constraints.


postgres=# *CREATE TABLE lt(val int UNIQUE DEFERRABLE INITIALLY DEFERRED);*
CREATE TABLE
postgres=# CREATE FOREIGN TABLE ft1_lt (val int) SERVER loopback1 
OPTIONS (table_name 'lt');

CREATE FOREIGN TABLE
postgres=# CREATE FOREIGN TABLE ft2_lt (val int) SERVER loopback2 
OPTIONS (table_name 'lt');

CREATE FOREIGN TABLE

*Test 1: **
**===*
In a transaction insert two rows one each to the two foreign tables and 
it works fine.


postgres=# BEGIN;
BEGIN
postgres=# INSERT INTO ft1_lt VALUES (1);
INSERT 0 1
postgres=# INSERT INTO ft2_lt VALUES (3);
INSERT 0 1
postgres=# COMMIT;
COMMIT
postgres=# SELECT * FROM lt;
 val
-
   1
   3
(2 rows)

*Test 2:**
**===*
In a transaction insert two rows one each to the two foreign tables.
One of the rows violates the constraint and other not. At the time of 
COMMIT one of the foreign server violates the constraints so it return 
error. I think this is also expected behavior.

postgres=# BEGIN;
BEGIN
postgres=# INSERT INTO ft1_lt VALUES (1); -- Violates constraint
INSERT 0 1
postgres=# INSERT INTO ft2_lt VALUES (4);
INSERT 0 1
postgres=# COMMIT;
2017-02-03 15:26:28.667 JST [3081] ERROR:  duplicate key value violates 
unique constraint "lt_val_key"

2017-02-03 15:26:28.667 JST [3081] DETAIL:  Key (val)=(1) already exists.
2017-02-03 15:26:28.667 JST [3081] STATEMENT:  COMMIT TRANSACTION
2017-02-03 15:26:28.668 JST [3075] ERROR:  duplicate key value violates 
unique constraint "lt_val_key"

2017-02-03 15:26:28.668 JST [3075] DETAIL:  Key (val)=(1) already exists.
2017-02-03 15:26:28.668 JST [3075] CONTEXT:  Remote SQL command: COMMIT 
TRANSACTION

2017-02-03 15:26:28.668 JST [3075] STATEMENT:  COMMIT;
2017-02-03 15:26:28.668 JST [3081] WARNING:  there is no transaction in 
progress

WARNING:  there is no transaction in progress
ERROR:  duplicate key value violates unique constraint "lt_val_key"
DETAIL:  Key (val)=(1) already exists.
CONTEXT:  Remote SQL command: COMMIT TRANSACTION
postgres=#
postgres=#
postgres=# SELECT * FROM lt;
 val
-
   1
   3
(2 rows)

*Test 3:**
**===*
In a transaction insert two rows one each to the two foreign tables.
One of the rows violates the constraint and other not. At the time of 
COMMIT one of the foreign server violates the constraints so it return 
error. I think this is also expected behavior.

postgres=# BEGIN;
BEGIN
postgres=# INSERT INTO ft1_lt VALUES (4);
INSERT 0 1
postgres=# INSERT INTO ft2_lt VALUES (3); -- Violates constraint
INSERT 0 1
postgres=# COMMIT;
2017-02-03 15:27:14.331 JST [3084] ERROR:  duplicate key value violates 
unique constraint "lt_val_key"

2017-02-03 15:27:14.331 JST [3084] DETAIL:  Key (val)=(3) already exists.
2017-02-03 15:27:14.331 JST [3084] STATEMENT:  COMMIT TRANSACTION
2017-02-03 15:27:14.332 JST [3075] ERROR:  duplicate key value violates 
unique constraint "lt_val_key"

2017-02-03 15:27:14.332 JST [3075] DETAIL:  Key (val)=(3) already exists.
2017-02-03 15:27:14.332 JST [3075] CONTEXT:  Remote SQL command: COMMIT 
TRANSACTION

2017-02-03 15:27:14.332 JST [3075] STATEMENT:  COMMIT;
2017-02-03 15:27:14.332 JST [3084] WARNING:  there is no transaction in 
progress

WARNING:  there is no transaction in progress
ERROR:  duplicate key value violates unique constraint "lt_val_key"
DETAIL:  Key (val)=(3) already exists.
CONTEXT:  Remote SQL command: COMMIT TRANSACTION
postgres=# SELECT * FROM lt;
 val
-
   1
   3
   4
(3 rows)
*Test 4:**
**===*
In a transaction insert two rows one each to the two foreign tables.
Both the rows violates the constraint. So at the time of COMMIT it 
returns error. I think this is also expected behavior.


postgres=# BEGIN;
BEGIN
postgres=# INSERT INTO ft1_lt VALUES (1); -- Violates constraint
INSERT 0 1
postgres=# INSERT INTO ft2_lt VALUES (3); -- Violates constraint
INSERT 0 1
postgres=# COMMIT;
2017-02-03 15:29:18.857 JST [3081] ERROR:  duplicate key value violates 
unique constraint "lt_val_key"

2017-02-03 15:29:18.857 JST [3081] DETAIL:  Key (val)=(1) already exists.
2017-02-03 

Re: [HACKERS] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/23 6:16), Tom Lane wrote:
 I committed this with some cosmetic adjustments, and one not-so-cosmetic
 one:

Thanks for improving and committing the patch!

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] postgres_fdw behaves oddly

[Ashutosh Bapat]

On Sun, Nov 23, 2014 at 2:46 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
  (2014/11/19 18:21), Ashutosh Bapat wrote:
  Ok. I added that comment to the commitfest and changed the status to
  ready for commiter.

  Many thanks!

 I committed this with some cosmetic adjustments, and one not-so-cosmetic
 one: I left it continuing to allow CTID conditions to be sent to the
 remote server.  That works today and we shouldn't disable it, especially
 not in the back branches where it could mean a performance regression
 in a minor release.


Thanks.


 As for the question of whether reltargetlist or tlist should be examined,
 reltargetlist is the correct thing.  I do not see a need to modify
 use_physical_tlist either.  If you trace through what happens in e.g.
 postgres_fdw, you'll notice that it only bothers to retrieve actually-used
 columns (which it computes correctly) from the remote side.  It then
 constructs a scan tuple that corresponds to the foreign table's physical
 column set, inserting NULLs for any unfetched columns.  This is handed
 back to execScan.c and matches what a heapscan or indexscan would produce.
 The point of the use_physical_tlist optimization is to avoid a projection
 step *on this tuple* if we don't really need to do one (ie, if it'd be
 just as good for the scan node's output tuple to be identical to the row's
 physical tuple).  If we disabled use_physical_tlist then we'd be forcing
 a projection step that would have the effect of removing the NULLs for
 unfetched columns, but it would not actually be of value, just as it isn't
 in the corresponding cases for heap/index scans.

 For the current patch, that's fair.

In grouping_planner() the targetlist of the topmost plan node of join tree
is changed to the set of expressions requested in the query.
1554 else
1555 {
1556 /*
1557  * Otherwise, just replace the subplan's flat
tlist with
1558  * the desired tlist.
1559  */
1560 result_plan-targetlist = sub_tlist;
1561 }
This means that for a simple SELECT from the foreign table, projection
would be necessary in ExecScan if all the columns are not required.

So, the strategy to emulate heap or index scan wastes extra cycles and
memory in adding NULL columns which are not required and then filtering
them during the projection (within ExecScan itself). It also limits the
ability of foreign scans to fetch shippable expressions when those
expressions are part of the targetlist and fetching their values (instead
of the columns participating in the expressions) reduces the size of the
fetched row.

These cases do not exist for heap scans or index scans because the tuples
are obtained directly from the buffer, so no extra memory required for the
extra columns and projection as well as expression evaluation is anyway
required.


BTW, given that there wasn't any very good way to test the createplan.c
 fix except by adding test cases to postgres_fdw, I didn't see much value
 in splitting the patch in two.  The committed patch includes tests that
 the createplan.c bug is fixed as well as the postgres_fdw one.

 regards, tom lane




-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] postgres_fdw behaves oddly

[Tom Lane]

Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 (2014/11/19 18:21), Ashutosh Bapat wrote:
 Ok. I added that comment to the commitfest and changed the status to
 ready for commiter.

 Many thanks!

I committed this with some cosmetic adjustments, and one not-so-cosmetic
one: I left it continuing to allow CTID conditions to be sent to the
remote server.  That works today and we shouldn't disable it, especially
not in the back branches where it could mean a performance regression
in a minor release.

As for the question of whether reltargetlist or tlist should be examined,
reltargetlist is the correct thing.  I do not see a need to modify
use_physical_tlist either.  If you trace through what happens in e.g.
postgres_fdw, you'll notice that it only bothers to retrieve actually-used
columns (which it computes correctly) from the remote side.  It then
constructs a scan tuple that corresponds to the foreign table's physical
column set, inserting NULLs for any unfetched columns.  This is handed
back to execScan.c and matches what a heapscan or indexscan would produce.
The point of the use_physical_tlist optimization is to avoid a projection
step *on this tuple* if we don't really need to do one (ie, if it'd be
just as good for the scan node's output tuple to be identical to the row's
physical tuple).  If we disabled use_physical_tlist then we'd be forcing
a projection step that would have the effect of removing the NULLs for
unfetched columns, but it would not actually be of value, just as it isn't
in the corresponding cases for heap/index scans.

BTW, given that there wasn't any very good way to test the createplan.c
fix except by adding test cases to postgres_fdw, I didn't see much value
in splitting the patch in two.  The committed patch includes tests that
the createplan.c bug is fixed as well as the postgres_fdw one.

regards, tom lane


-- 
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] postgres_fdw behaves oddly

[Ashutosh Bapat]

Ok. I added that comment to the commitfest and changed the status to ready
for commiter.

On Wed, Nov 19, 2014 at 1:10 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/11/19 15:56), Ashutosh Bapat wrote:

 On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
 (2014/11/19 14:58), Ashutosh Bapat wrote:


  May be we should modify use_physical_tlist() to return
 false in
 case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
 create_foreignscan_plan(). I do not see any create_*_plan()
 function
 using reltargetlist directly.


  Yeah, I think we can do that, but I'm not sure that we should use
 tlist in create_foreignscan_plan(), not rel-reltargetlist.  How
 about leaving this for committers to decide.


  I am fine with that. May be you want to add an XXX comment there to
 bring it to the committer's notice.


 It's ok, but I'm not convinced with your idea.  So, I think the comment
 can be adequately described by you, not by me.  So, my proposal is for you
 to add the comment to the CF app.  Could you do that?


 Thanks,

 Best regards,
 Etsuro Fujita




-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/19 18:21), Ashutosh Bapat wrote:

Ok. I added that comment to the commitfest and changed the status to
ready for commiter.


Many thanks!

PS: the link to the review emails that discuss the issue doesn't work 
properly, so I re-added the link.


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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/17 19:36), Ashutosh Bapat wrote:

Here are my comments about the patch fscan_reltargetlist.patch


Thanks for the review!


1. This isn't your change, but we might be able to get rid of assignment
2071 /* Are any system columns requested from rel? */
2072 scan_plan-fsSystemCol = false;

since make_foreignscan() already does that. But I will leave upto you to
remove this assignment or not.


As you pointed out, the assignment is redundant, but I think that that'd 
improve the clarity and readability.  So, I'd vote for leaving that as is.



2. Instead of using rel-reltargetlist, we should use the tlist passed
by caller. This is the tlist expected from the Plan node. For foreign
scans it will be same as rel-reltargetlist. Using tlist would make the
function consistent with create_*scan_plan functions.


I disagree with that for the reasons mentioned below:

* For a foreign scan, tlist is *not* necessarily the same as 
rel-reltargetlist (ie, there is a case where tlist contains all user 
attributes while rel-reltargetlist contains only attributes actually 
needed by the query).  In such a case it'd be inefficient to use tlist 
rather than rel-reltargetlist.


* I think that it'd improve the readability to match the code with other 
places that execute similar processing, such as check_index_only() and 
remove_unused_subquery_outputs().


Thanks,

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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/17 19:54), Ashutosh Bapat wrote:

Here are comments for postgres_fdw-syscol patch.


Thanks for the review!


Code
---
1. Instead of a single liner comment System columns can't be sent to
remote., it might be better to explain why system columns can't be sent
to the remote.


Done.


2. The comment in deparseVar is single line comment, so it should start
and end on the same line i.e. /* and */ should be on the same line.


Done.


3. Since there is already a testcase which triggered this particular
change, it will good, if we add that to regression in postgres_fdw.


Done.

Please find attached an updated version of the patch.

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 253,258  foreign_expr_walker(Node *node,
--- 253,268 
  if (var-varno == glob_cxt-foreignrel-relid 
  	var-varlevelsup == 0)
  {
+ 	/*
+ 	 * System columns can't be sent to remote since the values
+ 	 * for system columns might be different between local and
+ 	 * remote.
+ 	 *
+ 	 * XXX: we should probably send ctid to remote.
+ 	 */
+ 	if (var-varattno  0)
+ 		return false;
+ 
  	/* Var belongs to foreign table */
  	collation = var-varcollid;
  	state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
***
*** 1262,1267  deparseVar(Var *node, deparse_expr_cxt *context)
--- 1272,1280 
  	if (node-varno == context-foreignrel-relid 
  		node-varlevelsup == 0)
  	{
+ 		/* Var shouldn't be a system column */
+ 		Assert(node-varattno = 0);
+ 
  		/* Var belongs to foreign table */
  		deparseColumnRef(buf, node-varno, node-varattno, context-root);
  	}
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 353,358  EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
--- 353,368 
 Remote SQL: SELECT C 1, c2, c3, c4, c5, c6, c7, c8 FROM S 1.T 1 WHERE ((C 1 = c2))
  (3 rows)
  
+ -- where with system column conditions
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.tableoid != 0;
+QUERY PLAN
+ -
+  Foreign Scan on public.ft1 t1
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+Filter: (t1.tableoid  0::oid)
+Remote SQL: SELECT C 1, c2, c3, c4, c5, c6, c7, c8 FROM S 1.T 1
+ (4 rows)
+ 
  -- ===
  -- WHERE with remotely-executable conditions
  -- ===
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***
*** 180,185  EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = postgres_fdw_a
--- 180,187 
  EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2;
  EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = abs(t1.c2);
  EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
+ -- where with system column conditions
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.tableoid != 0;
  
  -- ===
  -- WHERE with remotely-executable conditions

-- 
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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/18 18:27), Ashutosh Bapat wrote:

On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/17 19:36), Ashutosh Bapat wrote:



Here are my comments about the patch fscan_reltargetlist.patch



2. Instead of using rel-reltargetlist, we should use the tlist
passed
by caller. This is the tlist expected from the Plan node. For
foreign
scans it will be same as rel-reltargetlist. Using tlist would
make the
function consistent with create_*scan_plan functions.



I disagree with that for the reasons mentioned below:

* For a foreign scan, tlist is *not* necessarily the same as
rel-reltargetlist (ie, there is a case where tlist contains all
user attributes while rel-reltargetlist contains only attributes
actually needed by the query).  In such a case it'd be inefficient
to use tlist rather than rel-reltargetlist.



create_foreignscan_plan() is called from create_scan_plan(), which
passes the tlist. The later uses function use_physical_tlist() to decide
which targetlist should be used (physical or actual). As per code below
in this function

  485 /*
  486  * We can do this for real relation scans, subquery scans,
function scans,
  487  * values scans, and CTE scans (but not for, eg, joins).
  488  */
  489 if (rel-rtekind != RTE_RELATION 
  490 rel-rtekind != RTE_SUBQUERY 
  491 rel-rtekind != RTE_FUNCTION 
  492 rel-rtekind != RTE_VALUES 
  493 rel-rtekind != RTE_CTE)
  494 return false;
  495
  496 /*
  497  * Can't do it with inheritance cases either (mainly because
Append
  498  * doesn't project).
  499  */
  500 if (rel-reloptkind != RELOPT_BASEREL)
  501 return false;

For foreign tables as well as the tables under inheritance hierarchy it
uses the actual targetlist, which contains only the required columns IOW
rel-reltargetlist (see build_path_tlist()) with nested loop parameters
substituted. So, it never included unnecessary columns in the
targetlist.


Maybe I'm missing something, but I think you are overlooking the case 
for foreign tables that are *not* under an inheritance hierarchy.  (Note 
that the rtekind for foreign tables is RTE_RELATION.)


Thanks,

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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/18 18:37), Ashutosh Bapat wrote:

On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/17 19:54), Ashutosh Bapat wrote:



Here are comments for postgres_fdw-syscol patch.



Code
---
1. Instead of a single liner comment System columns can't be
sent to
remote., it might be better to explain why system columns can't
be sent
to the remote.



Done.



I would add  and foreign values do not make sense locally (except may
be ctid clubbed with foreign server_id) to make it more clear. But I
will leave that for the commiter to decide.


OK.


3. Since there is already a testcase which triggered this particular
change, it will good, if we add that to regression in postgres_fdw.



Done.



I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE
t1.tableoid = t2.oid. The condition makes sure that the tableoid in the
row is same as the OID of the foreign table recorded in pg_class
locally. And the EXPLAIN of the query which clearly shows that the
tableoid column in not fetched from the foreign server.


I thought that test, but I didn't use it because I think we can't see 
(at least from the EXPLAIN) why the qual is not pushed down: the qual 
isn't pushed down possibly becasue the qual is considered as a *join* 
qual, not because the qual just cotains tableoid.  (Having said that, I 
think we can see if the qual isn't pushed down as a join qual for a 
parameterized plan, but ISTM it's worth complicating regression tests.)



Once we resolve the other patch on this thread,  I think this item can
be marked as ready for commiter from my side.


OK.  Thank you for the review.

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] postgres_fdw behaves oddly

[Ashutosh Bapat]

On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
 wrote:

 (2014/11/18 18:27), Ashutosh Bapat wrote:

 On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
 (2014/11/17 19:36), Ashutosh Bapat wrote:


  Here are my comments about the patch fscan_reltargetlist.patch


  2. Instead of using rel-reltargetlist, we should use the tlist
 passed
 by caller. This is the tlist expected from the Plan node. For
 foreign
 scans it will be same as rel-reltargetlist. Using tlist would
 make the
 function consistent with create_*scan_plan functions.


  I disagree with that for the reasons mentioned below:

 * For a foreign scan, tlist is *not* necessarily the same as
 rel-reltargetlist (ie, there is a case where tlist contains all
 user attributes while rel-reltargetlist contains only attributes
 actually needed by the query).  In such a case it'd be inefficient
 to use tlist rather than rel-reltargetlist.


  create_foreignscan_plan() is called from create_scan_plan(), which
 passes the tlist. The later uses function use_physical_tlist() to decide
 which targetlist should be used (physical or actual). As per code below
 in this function

   485 /*
   486  * We can do this for real relation scans, subquery scans,
 function scans,
   487  * values scans, and CTE scans (but not for, eg, joins).
   488  */
   489 if (rel-rtekind != RTE_RELATION 
   490 rel-rtekind != RTE_SUBQUERY 
   491 rel-rtekind != RTE_FUNCTION 
   492 rel-rtekind != RTE_VALUES 
   493 rel-rtekind != RTE_CTE)
   494 return false;
   495
   496 /*
   497  * Can't do it with inheritance cases either (mainly because
 Append
   498  * doesn't project).
   499  */
   500 if (rel-reloptkind != RELOPT_BASEREL)
   501 return false;

 For foreign tables as well as the tables under inheritance hierarchy it
 uses the actual targetlist, which contains only the required columns IOW
 rel-reltargetlist (see build_path_tlist()) with nested loop parameters
 substituted. So, it never included unnecessary columns in the
 targetlist.


 Maybe I'm missing something, but I think you are overlooking the case for
 foreign tables that are *not* under an inheritance hierarchy.  (Note that
 the rtekind for foreign tables is RTE_RELATION.)



Ah! you are right. I confused between rtekind and relkind. Sorry for that.
May be we should modify use_physical_tlist() to return false in case of
RELKIND_FOREIGN_TABLE, so that we can use tlist in
create_foreignscan_plan(). I do not see any create_*_plan() function using
reltargetlist directly.


 Thanks,

 Best regards,
 Etsuro Fujita




-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/19 14:58), Ashutosh Bapat wrote:

On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/18 18:27), Ashutosh Bapat wrote:
On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp
 (2014/11/17 19:36), Ashutosh Bapat wrote:



 Here are my comments about the patch
fscan_reltargetlist.patch



 2. Instead of using rel-reltargetlist, we should use
the tlist
 passed
 by caller. This is the tlist expected from the Plan
node. For
 foreign
 scans it will be same as rel-reltargetlist. Using
tlist would
 make the
 function consistent with create_*scan_plan functions.



 I disagree with that for the reasons mentioned below:

 * For a foreign scan, tlist is *not* necessarily the same as
 rel-reltargetlist (ie, there is a case where tlist
contains all
 user attributes while rel-reltargetlist contains only
attributes
 actually needed by the query).  In such a case it'd be
inefficient
 to use tlist rather than rel-reltargetlist.



create_foreignscan_plan() is called from create_scan_plan(), which
passes the tlist. The later uses function use_physical_tlist()
to decide
which targetlist should be used (physical or actual). As per
code below
in this function

   485 /*
   486  * We can do this for real relation scans, subquery
scans,
function scans,
   487  * values scans, and CTE scans (but not for, eg, joins).
   488  */
   489 if (rel-rtekind != RTE_RELATION 
   490 rel-rtekind != RTE_SUBQUERY 
   491 rel-rtekind != RTE_FUNCTION 
   492 rel-rtekind != RTE_VALUES 
   493 rel-rtekind != RTE_CTE)
   494 return false;
   495
   496 /*
   497  * Can't do it with inheritance cases either (mainly
because
Append
   498  * doesn't project).
   499  */
   500 if (rel-reloptkind != RELOPT_BASEREL)
   501 return false;

For foreign tables as well as the tables under inheritance
hierarchy it
uses the actual targetlist, which contains only the required
columns IOW
rel-reltargetlist (see build_path_tlist()) with nested loop
parameters
substituted. So, it never included unnecessary columns in the
targetlist.



Maybe I'm missing something, but I think you are overlooking the
case for foreign tables that are *not* under an inheritance
hierarchy.  (Note that the rtekind for foreign tables is RTE_RELATION.)



Ah! you are right. I confused between rtekind and relkind. Sorry for
that. May be we should modify use_physical_tlist() to return false in
case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
create_foreignscan_plan(). I do not see any create_*_plan() function
using reltargetlist directly.


Yeah, I think we can do that, but I'm not sure that we should use tlist 
in create_foreignscan_plan(), not rel-reltargetlist.  How about leaving 
this for committers to decide.


Thanks,

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] postgres_fdw behaves oddly

[Ashutosh Bapat]

On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
 wrote:

 (2014/11/19 14:58), Ashutosh Bapat wrote:

 On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
 (2014/11/18 18:27), Ashutosh Bapat wrote:
 On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp
  (2014/11/17 19:36), Ashutosh Bapat wrote:


   Here are my comments about the patch
 fscan_reltargetlist.patch


   2. Instead of using rel-reltargetlist, we should use
 the tlist
  passed
  by caller. This is the tlist expected from the Plan
 node. For
  foreign
  scans it will be same as rel-reltargetlist. Using
 tlist would
  make the
  function consistent with create_*scan_plan functions.


   I disagree with that for the reasons mentioned below:

  * For a foreign scan, tlist is *not* necessarily the same as
  rel-reltargetlist (ie, there is a case where tlist
 contains all
  user attributes while rel-reltargetlist contains only
 attributes
  actually needed by the query).  In such a case it'd be
 inefficient
  to use tlist rather than rel-reltargetlist.


  create_foreignscan_plan() is called from create_scan_plan(), which
 passes the tlist. The later uses function use_physical_tlist()
 to decide
 which targetlist should be used (physical or actual). As per
 code below
 in this function

485 /*
486  * We can do this for real relation scans, subquery
 scans,
 function scans,
487  * values scans, and CTE scans (but not for, eg,
 joins).
488  */
489 if (rel-rtekind != RTE_RELATION 
490 rel-rtekind != RTE_SUBQUERY 
491 rel-rtekind != RTE_FUNCTION 
492 rel-rtekind != RTE_VALUES 
493 rel-rtekind != RTE_CTE)
494 return false;
495
496 /*
497  * Can't do it with inheritance cases either (mainly
 because
 Append
498  * doesn't project).
499  */
500 if (rel-reloptkind != RELOPT_BASEREL)
501 return false;

 For foreign tables as well as the tables under inheritance
 hierarchy it
 uses the actual targetlist, which contains only the required
 columns IOW
 rel-reltargetlist (see build_path_tlist()) with nested loop
 parameters
 substituted. So, it never included unnecessary columns in the
 targetlist.


  Maybe I'm missing something, but I think you are overlooking the
 case for foreign tables that are *not* under an inheritance
 hierarchy.  (Note that the rtekind for foreign tables is
 RTE_RELATION.)


  Ah! you are right. I confused between rtekind and relkind. Sorry for
 that. May be we should modify use_physical_tlist() to return false in
 case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
 create_foreignscan_plan(). I do not see any create_*_plan() function
 using reltargetlist directly.


 Yeah, I think we can do that, but I'm not sure that we should use tlist in
 create_foreignscan_plan(), not rel-reltargetlist.  How about leaving this
 for committers to decide.


I am fine with that. May be you want to add an XXX comment there to bring
it to the committer's notice.



 Thanks,

 Best regards,
 Etsuro Fujita




-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/19 14:55), Etsuro Fujita wrote:

(2014/11/18 18:37), Ashutosh Bapat wrote:

On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/17 19:54), Ashutosh Bapat wrote:



Here are comments for postgres_fdw-syscol patch.



3. Since there is already a testcase which triggered this
particular
change, it will good, if we add that to regression in
postgres_fdw.



Done.



I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE
t1.tableoid = t2.oid. The condition makes sure that the tableoid in the
row is same as the OID of the foreign table recorded in pg_class
locally. And the EXPLAIN of the query which clearly shows that the
tableoid column in not fetched from the foreign server.


I thought that test, but I didn't use it because I think we can't see
(at least from the EXPLAIN) why the qual is not pushed down: the qual
isn't pushed down possibly becasue the qual is considered as a *join*
qual, not because the qual just cotains tableoid.  (Having said that, I
think we can see if the qual isn't pushed down as a join qual for a
parameterized plan, but ISTM it's worth complicating regression tests.)


Sorry, I incorrectly wrote the last sentence.  What I tried to write is, 
ISTM it's *not* worth complicating regression tests.


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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/19 15:56), Ashutosh Bapat wrote:

On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/19 14:58), Ashutosh Bapat wrote:



May be we should modify use_physical_tlist() to return
false in
case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
create_foreignscan_plan(). I do not see any create_*_plan() function
using reltargetlist directly.



Yeah, I think we can do that, but I'm not sure that we should use
tlist in create_foreignscan_plan(), not rel-reltargetlist.  How
about leaving this for committers to decide.



I am fine with that. May be you want to add an XXX comment there to
bring it to the committer's notice.


It's ok, but I'm not convinced with your idea.  So, I think the comment 
can be adequately described by you, not by me.  So, my proposal is for 
you to add the comment to the CF app.  Could you do that?


Thanks,

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] postgres_fdw behaves oddly

[Ashutosh Bapat]

Hi Fujita-san,

Here are comments for postgres_fdw-syscol patch.

Sanity

The patch applies and compiles cleanly.
The server regression and regression in contrib/postgres_fdw,file_fdw run
cleanly.

Code
---
1. Instead of a single liner comment System columns can't be sent to
remote., it might be better to explain why system columns can't be sent to
the remote.
2. The comment in deparseVar is single line comment, so it should start and
end on the same line i.e. /* and */ should be on the same line.
3. Since there is already a testcase which triggered this particular
change, it will good, if we add that to regression in postgres_fdw.

On Mon, Nov 17, 2014 at 4:06 PM, Ashutosh Bapat 
ashutosh.ba...@enterprisedb.com wrote:

 Hi Fujita-san,

 Here are my comments about the patch fscan_reltargetlist.patch
 Sanity
 
 Patch applies and compiles cleanly.
 Regressions in test/regress folder and postgres_fdw and file_fdw are clean.

 1. This isn't your change, but we might be able to get rid of assignment
 2071 /* Are any system columns requested from rel? */
 2072 scan_plan-fsSystemCol = false;

 since make_foreignscan() already does that. But I will leave upto you to
 remove this assignment or not.

 2. Instead of using rel-reltargetlist, we should use the tlist passed by
 caller. This is the tlist expected from the Plan node. For foreign scans it
 will be same as rel-reltargetlist. Using tlist would make the function
 consistent with create_*scan_plan functions.

 Otherwise, the patch looks good.

 On Wed, Nov 12, 2014 at 12:55 PM, Etsuro Fujita 
 fujita.ets...@lab.ntt.co.jp wrote:

 (2014/11/11 18:45), Etsuro Fujita wrote:

 (2014/11/10 20:05), Ashutosh Bapat wrote:

 Since two separate issues 1. using reltargetlist instead of attr_needed
 and 2. system columns usage in FDW are being tackled here, we should
 separate the patch into two one for each of the issues.


 Agreed.  Will split the patch into two.


 Here are splitted patches:

 fscan-reltargetlist.patch  - patch for #1
 postgres_fdw-syscol.patch  - patch for #2


 Thanks,

 Best regards,
 Etsuro Fujita




 --
 Best Wishes,
 Ashutosh Bapat
 EnterpriseDB Corporation
 The Postgres Database Company




-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] postgres_fdw behaves oddly

[Etsuro Fujita]

Hi Ashutosh,

Thanks for the review!

(2014/11/10 20:05), Ashutosh Bapat wrote:

Since two separate issues 1. using reltargetlist instead of attr_needed
and 2. system columns usage in FDW are being tackled here, we should
separate the patch into two one for each of the issues.


Agreed.  Will split the patch into two.


While I agree that the system columns shouldn't be sent to the remote
node, it doesn't look clear to me as to what would they or their values
mean in the context of foreign data. Some columns like tableoid would
have a value which is the OID of the foreign table, other system columns
like xmin/xmax/ctid will have different meanings (or no meaning)
depending upon the foreign data source. In case of later columns, each
FDW would have its own way of defining that meaning (I guess). But in
any case, I agree that we shouldn't send the system columns to the
remote side.

Is there a way we can enforce this rule across all the FDWs? OR we want
to tackle it separately per FDW?


ISTM it'd be better to tackle it separately because I feel that such an 
enforcement by the PG core would go too far.  I'm also concerned about a 
possibility of impedance mismatching between such an enforcement and 
postgres_fdw, which sends the ctid column to the remote side internally 
when executing UPDATE/DELETE on foreign tables.  See 
postgresPlanForeignModify and eg, deparseUpdateSql.


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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/11/11 18:45), Etsuro Fujita wrote:

(2014/11/10 20:05), Ashutosh Bapat wrote:

Since two separate issues 1. using reltargetlist instead of attr_needed
and 2. system columns usage in FDW are being tackled here, we should
separate the patch into two one for each of the issues.


Agreed.  Will split the patch into two.


Here are splitted patches:

fscan-reltargetlist.patch  - patch for #1
postgres_fdw-syscol.patch  - patch for #2

Thanks,

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 20,25 
--- 20,26 
  #include math.h
  
  #include access/skey.h
+ #include access/sysattr.h
  #include catalog/pg_class.h
  #include foreign/fdwapi.h
  #include miscadmin.h
***
*** 1945,1950  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
--- 1946,1953 
  	RelOptInfo *rel = best_path-path.parent;
  	Index		scan_relid = rel-relid;
  	RangeTblEntry *rte;
+ 	Bitmapset  *attrs_used = NULL;
+ 	ListCell   *lc;
  	int			i;
  
  	/* it should be a base rel... */
***
*** 1993,2008  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
  	return scan_plan;
  }
  
--- 1996,2030 
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
+ 
+ 	/*
+ 	 * Add all the attributes needed for joins or final output.  Note: we must
+ 	 * look at reltargetlist, not the attr_needed data, because attr_needed
+ 	 * isn't computed for inheritance child rels.
+ 	 */
+ 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
+ 
+ 	/* Add all the attributes used by restriction clauses. */
+ 	foreach(lc, rel-baserestrictinfo)
+ 	{
+ 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+ 
+ 		pull_varattnos((Node *) rinfo-clause, rel-relid, attrs_used);
+ 	}
+ 
+ 	/* Are any system columns requested from rel? */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
+ 	bms_free(attrs_used);
+ 
  	return scan_plan;
  }
  
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 252,257  foreign_expr_walker(Node *node,
--- 252,265 
  if (var-varno == glob_cxt-foreignrel-relid 
  	var-varlevelsup == 0)
  {
+ 	/*
+ 	 * System columns can't be sent to remote.
+ 	 *
+ 	 * XXX: we should probably send ctid to remote.
+ 	 */
+ 	if (var-varattno  0)
+ 		return false;
+ 
  	/* Var belongs to foreign table */
  	collation = var-varcollid;
  	state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
***
*** 1261,1266  deparseVar(Var *node, deparse_expr_cxt *context)
--- 1269,1279 
  	if (node-varno == context-foreignrel-relid 
  		node-varlevelsup == 0)
  	{
+ 		/*
+ 		 * System columns shouldn't be examined here.
+ 		 */
+ 		Assert(node-varattno = 0);
+ 
  		/* Var belongs to foreign table */
  		deparseColumnRef(buf, node-varno, node-varattno, context-root);
  	}

-- 
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] postgres_fdw behaves oddly

[Ashutosh Bapat]

Since two separate issues 1. using reltargetlist instead of attr_needed and
2. system columns usage in FDW are being tackled here, we should separate
the patch into two one for each of the issues.

While I agree that the system columns shouldn't be sent to the remote node,
it doesn't look clear to me as to what would they or their values mean in
the context of foreign data. Some columns like tableoid would have a value
which is the OID of the foreign table, other system columns like
xmin/xmax/ctid will have different meanings (or no meaning) depending upon
the foreign data source. In case of later columns, each FDW would have its
own way of defining that meaning (I guess). But in any case, I agree that
we shouldn't send the system columns to the remote side.

Is there a way we can enforce this rule across all the FDWs? OR we want to
tackle it separately per FDW?

On Tue, Oct 14, 2014 at 8:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/10/14 8:53), Robert Haas wrote:

 On Mon, Oct 13, 2014 at 3:30 PM, Bruce Momjian br...@momjian.us wrote:

 Uh, where are we on this patch?


 Probably it should be added to the upcoming CF.


 Done.

 https://commitfest.postgresql.org/action/patch_view?id=1599

 Thanks,

 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




-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] postgres_fdw behaves oddly

[Bruce Momjian]

Uh, where are we on this patch?

---

On Mon, Sep  8, 2014 at 09:30:32PM +0900, Etsuro Fujita wrote:
 (2014/09/02 18:55), Etsuro Fujita wrote:
  (2014/09/01 20:15), Etsuro Fujita wrote:
  While working on [1], I've found that postgres_fdw behaves oddly:
 
 I've revised the patch a bit further.  Please find attached a patch.
 
 Thanks,
 
 Best regards,
 Etsuro Fujita

 *** a/contrib/postgres_fdw/deparse.c
 --- b/contrib/postgres_fdw/deparse.c
 ***
 *** 252,257  foreign_expr_walker(Node *node,
 --- 252,265 
   if (var-varno == glob_cxt-foreignrel-relid 
   var-varlevelsup == 0)
   {
 + /*
 +  * System columns can't be sent to 
 remote.
 +  *
 +  * XXX: we should probably send ctid to 
 remote.
 +  */
 + if (var-varattno  0)
 + return false;
 + 
   /* Var belongs to foreign table */
   collation = var-varcollid;
   state = OidIsValid(collation) ? 
 FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
 ***
 *** 1261,1266  deparseVar(Var *node, deparse_expr_cxt *context)
 --- 1269,1279 
   if (node-varno == context-foreignrel-relid 
   node-varlevelsup == 0)
   {
 + /*
 +  * System columns shouldn't be examined here.
 +  */
 + Assert(node-varattno = 0);
 + 
   /* Var belongs to foreign table */
   deparseColumnRef(buf, node-varno, node-varattno, 
 context-root);
   }
 *** a/src/backend/optimizer/plan/createplan.c
 --- b/src/backend/optimizer/plan/createplan.c
 ***
 *** 20,25 
 --- 20,26 
   #include math.h
   
   #include access/skey.h
 + #include access/sysattr.h
   #include catalog/pg_class.h
   #include foreign/fdwapi.h
   #include miscadmin.h
 ***
 *** 1945,1950  create_foreignscan_plan(PlannerInfo *root, ForeignPath 
 *best_path,
 --- 1946,1953 
   RelOptInfo *rel = best_path-path.parent;
   Index   scan_relid = rel-relid;
   RangeTblEntry *rte;
 + Bitmapset  *attrs_used = NULL;
 + ListCell   *lc;
   int i;
   
   /* it should be a base rel... */
 ***
 *** 1993,2008  create_foreignscan_plan(PlannerInfo *root, ForeignPath 
 *best_path,
* bit of a kluge and might go away someday, so we intentionally leave 
 it
* out of the API presented to FDWs.
*/
   scan_plan-fsSystemCol = false;
   for (i = rel-min_attr; i  0; i++)
   {
 ! if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
   {
   scan_plan-fsSystemCol = true;
   break;
   }
   }
   
   return scan_plan;
   }
   
 --- 1996,2030 
* bit of a kluge and might go away someday, so we intentionally leave 
 it
* out of the API presented to FDWs.
*/
 + 
 + /*
 +  * Add all the attributes needed for joins or final output.  Note: we 
 must
 +  * look at reltargetlist, not the attr_needed data, because attr_needed
 +  * isn't computed for inheritance child rels.
 +  */
 + pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
 + 
 + /* Add all the attributes used by restriction clauses. */
 + foreach(lc, rel-baserestrictinfo)
 + {
 + RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
 + 
 + pull_varattnos((Node *) rinfo-clause, rel-relid, attrs_used);
 + }
 + 
 + /* Are any system columns requested from rel? */
   scan_plan-fsSystemCol = false;
   for (i = rel-min_attr; i  0; i++)
   {
 ! if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, 
 attrs_used))
   {
   scan_plan-fsSystemCol = true;
   break;
   }
   }
   
 + bms_free(attrs_used);
 + 
   return scan_plan;
   }
   

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


-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
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] postgres_fdw behaves oddly

[Robert Haas]

On Mon, Oct 13, 2014 at 3:30 PM, Bruce Momjian br...@momjian.us wrote:
 Uh, where are we on this patch?

Probably it should be added to the upcoming CF.

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


-- 
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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/09/02 18:55), Etsuro Fujita wrote:
 (2014/09/01 20:15), Etsuro Fujita wrote:
 While working on [1], I've found that postgres_fdw behaves oddly:

I've revised the patch a bit further.  Please find attached a patch.

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 252,257  foreign_expr_walker(Node *node,
--- 252,265 
  if (var-varno == glob_cxt-foreignrel-relid 
  	var-varlevelsup == 0)
  {
+ 	/*
+ 	 * System columns can't be sent to remote.
+ 	 *
+ 	 * XXX: we should probably send ctid to remote.
+ 	 */
+ 	if (var-varattno  0)
+ 		return false;
+ 
  	/* Var belongs to foreign table */
  	collation = var-varcollid;
  	state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
***
*** 1261,1266  deparseVar(Var *node, deparse_expr_cxt *context)
--- 1269,1279 
  	if (node-varno == context-foreignrel-relid 
  		node-varlevelsup == 0)
  	{
+ 		/*
+ 		 * System columns shouldn't be examined here.
+ 		 */
+ 		Assert(node-varattno = 0);
+ 
  		/* Var belongs to foreign table */
  		deparseColumnRef(buf, node-varno, node-varattno, context-root);
  	}
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 20,25 
--- 20,26 
  #include math.h
  
  #include access/skey.h
+ #include access/sysattr.h
  #include catalog/pg_class.h
  #include foreign/fdwapi.h
  #include miscadmin.h
***
*** 1945,1950  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
--- 1946,1953 
  	RelOptInfo *rel = best_path-path.parent;
  	Index		scan_relid = rel-relid;
  	RangeTblEntry *rte;
+ 	Bitmapset  *attrs_used = NULL;
+ 	ListCell   *lc;
  	int			i;
  
  	/* it should be a base rel... */
***
*** 1993,2008  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
  	return scan_plan;
  }
  
--- 1996,2030 
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
+ 
+ 	/*
+ 	 * Add all the attributes needed for joins or final output.  Note: we must
+ 	 * look at reltargetlist, not the attr_needed data, because attr_needed
+ 	 * isn't computed for inheritance child rels.
+ 	 */
+ 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
+ 
+ 	/* Add all the attributes used by restriction clauses. */
+ 	foreach(lc, rel-baserestrictinfo)
+ 	{
+ 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+ 
+ 		pull_varattnos((Node *) rinfo-clause, rel-relid, attrs_used);
+ 	}
+ 
+ 	/* Are any system columns requested from rel? */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
+ 	bms_free(attrs_used);
+ 
  	return scan_plan;
  }
  

-- 
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] postgres_fdw behaves oddly

[Etsuro Fujita]

(2014/09/01 20:15), Etsuro Fujita wrote:
 While working on [1], I've found that postgres_fdw behaves oddly:
 
 postgres=# create foreign table ft (a int) server loopback options
 (table_name 't');
 CREATE FOREIGN TABLE
 postgres=# select tableoid, * from ft;
   tableoid | a
 --+---
  16400 | 1
 (1 row)
 
 postgres=# select tableoid, * from ft where tableoid = 16400;
   tableoid | a
 --+---
 (0 rows)

 I think that one simple way of fixing such issues would be
 to consider unsafe to send to the remote a qual that contains any system
 columns.

I noticed the previous patch has overdone it.  Attached is an updated
version of the patch.

Thanks,

PS:
 [1] https://commitfest.postgresql.org/action/patch_view?id=1386

I'll update the patch in [1] on top of this version.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 252,257  foreign_expr_walker(Node *node,
--- 252,263 
  if (var-varno == glob_cxt-foreignrel-relid 
  	var-varlevelsup == 0)
  {
+ 	/*
+ 	 * System columns can't be sent to remote.
+ 	 */
+ 	if (var-varattno  0)
+ 		return false;
+ 
  	/* Var belongs to foreign table */
  	collation = var-varcollid;
  	state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 20,25 
--- 20,26 
  #include math.h
  
  #include access/skey.h
+ #include access/sysattr.h
  #include catalog/pg_class.h
  #include foreign/fdwapi.h
  #include miscadmin.h
***
*** 1945,1950  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
--- 1946,1953 
  	RelOptInfo *rel = best_path-path.parent;
  	Index		scan_relid = rel-relid;
  	RangeTblEntry *rte;
+ 	Bitmapset  *attrs_used = NULL;
+ 	ListCell   *lc;
  	int			i;
  
  	/* it should be a base rel... */
***
*** 1993,2008  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
  	return scan_plan;
  }
  
--- 1996,2030 
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
+ 
+ 	/*
+ 	 * Add all the attributes needed for joins or final output.  Note: we must
+ 	 * look at reltargetlist, not the attr_needed data, because attr_needed
+ 	 * isn't computed for inheritance child rels.
+ 	 */
+ 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
+ 
+ 	/* Add all the attributes used by restriction clauses. */
+ 	foreach(lc, rel-baserestrictinfo)
+ 	{
+ 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+ 
+ 		pull_varattnos((Node *) rinfo-clause, rel-relid, attrs_used);
+ 	}
+ 
+ 	/* Are any system columns requested from rel? */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
+ 	bms_free(attrs_used);
+ 
  	return scan_plan;
  }
  

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

[HACKERS] postgres_fdw behaves oddly

[Etsuro Fujita]

While working on [1], I've found that postgres_fdw behaves oddly:

postgres=# create foreign table ft (a int) server loopback options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# select tableoid, * from ft;
 tableoid | a
--+---
16400 | 1
(1 row)

postgres=# select tableoid, * from ft where tableoid = 16400;
 tableoid | a
--+---
(0 rows)

I think that this is because (a) the qual that contains tableoid can be
sent to the remote as shown in the EXPLAIN output:

postgres=# explain verbose select tableoid, * from ft where tableoid =
16400;
  QUERY PLAN
--
 Foreign Scan on public.ft  (cost=100.00..193.20 rows=2560 width=8)
   Output: tableoid, a
   Remote SQL: SELECT a FROM public.t WHERE ((tableoid = 16400::oid))
 Planning time: 0.110 ms
(4 rows)

and because (b) the tableoid value can be differs between the local and
the remote.  I think that one simple way of fixing such issues would be
to consider unsafe to send to the remote a qual that contains any system
columns (though we should probably give special treatment to quals
containing only ctid).  With the modification of postgres_fdw, we get
the right result:

postgres=# select tableoid, * from ft where tableoid = 16400;
 tableoid | a
--+---
16400 | 1
(1 row)

However, it's not complete enough.  Here is another surising result
(note no tableoid column in the select list):

postgres=# select * from ft where tableoid = 16400;
 a
---
(0 rows)

I think that this is because create_foreignscan_plan doesn't refer to
rel-baserestrictinfo when detecting whether any system columns are
requested.  By the additional modification of create_foreignscan_plan,
we get the right result:

postgres=# select * from ft where tableoid = 16400;
 a
---
 1
(1 row)

I'd also like to propose to change the function so as to make reference
to rel-reltargetlist, not to attr_needed, to match the code with other
places.  Please find attached a patch.

Thanks,

[1] https://commitfest.postgresql.org/action/patch_view?id=1386

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 243,248  foreign_expr_walker(Node *node,
--- 243,254 
  Var		   *var = (Var *) node;
  
  /*
+  * System columns can't be sent to remote.
+  */
+ if (var-varattno  0)
+ 	return false;
+ 
+ /*
   * If the Var is from the foreign table, we consider its
   * collation (if any) safe to use.  If it is from another
   * table, we treat its collation the same way as we would a
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 20,25 
--- 20,26 
  #include math.h
  
  #include access/skey.h
+ #include access/sysattr.h
  #include catalog/pg_class.h
  #include foreign/fdwapi.h
  #include miscadmin.h
***
*** 1945,1950  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
--- 1946,1953 
  	RelOptInfo *rel = best_path-path.parent;
  	Index		scan_relid = rel-relid;
  	RangeTblEntry *rte;
+ 	Bitmapset  *attrs_used = NULL;
+ 	ListCell   *lc;
  	int			i;
  
  	/* it should be a base rel... */
***
*** 1993,2008  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
  	return scan_plan;
  }
  
--- 1996,2030 
  	 * bit of a kluge and might go away someday, so we intentionally leave it
  	 * out of the API presented to FDWs.
  	 */
+ 
+ 	/*
+ 	 * Add all the attributes needed for joins or final output.  Note: we must
+ 	 * look at reltargetlist, not the attr_needed data, because attr_needed
+ 	 * isn't computed for inheritance child rels.
+ 	 */
+ 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
+ 
+ 	/* Add all the attributes used by restriction clauses. */
+ 	foreach(lc, rel-baserestrictinfo)
+ 	{
+ 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+ 
+ 		pull_varattnos((Node *) rinfo-clause, rel-relid, attrs_used);
+ 	}
+ 
+ 	/* Are any system columns requested from rel? */
  	scan_plan-fsSystemCol = false;
  	for (i = rel-min_attr; i  0; i++)
  	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
  			scan_plan-fsSystemCol = true;
  			break;
  		}
  	}
  
+ 	bms_free(attrs_used);
+ 
  	return scan_plan;
  }
  

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