Postgres_fdw join pushdown - wrong results with whole-row reference
Below query returns the wrong result when join getting pushdown to the
remote
server.
(PFA fdw_setup.sql, to create objects for the test)
postgres=# select e, e.empno, d.deptno, d.dname from f_emp e left join
f_dept d on e.deptno = d.deptno and e.sal > 3000 order by 2, 3 limit 10;
e | empno | deptno |
dname
-----------------------------------------------------------+-------+--------+------------
| 7369 |
|
(7499,ALLEN,SALESMAN,7698,1981-02-20,1600.00,300.00,30) | 7499 |
|
(7521,WARD,SALESMAN,7698,1981-02-22,1250.00,500.00,30) | 7521 |
|
| 7566 |
|
(7654,MARTIN,SALESMAN,7698,1981-09-28,1250.00,1400.00,30) | 7654 |
|
| 7698 |
|
| 7782 |
|
| 7788 |
|
| 7839 | 10
| ACCOUNTING
(7844,TURNER,SALESMAN,7698,1981-09-08,1500.00,0.00,30) | 7844 |
|
(10 rows)
Here, wholerow is coming as NULL even though with non-null empno. If we
remove
limit clause from the query - that will not push the query to the remote
side
and in such case getting correct output.
postgres=# select e, e.empno, d.deptno, d.dname from f_emp e left join
f_dept d on e.deptno = d.deptno and e.sal > 3000 order by 2, 3;
e | empno | deptno
| dname
-----------------------------------------------------------+-------+--------+------------
(7369,SMITH,CLERK,7902,1980-12-17,800.00,,20) | 7369 |
|
(7499,ALLEN,SALESMAN,7698,1981-02-20,1600.00,300.00,30) | 7499 |
|
(7521,WARD,SALESMAN,7698,1981-02-22,1250.00,500.00,30) | 7521 |
|
(7566,JONES,MANAGER,7839,1981-04-02,2975.00,,20) | 7566 |
|
(7654,MARTIN,SALESMAN,7698,1981-09-28,1250.00,1400.00,30) | 7654 |
|
(7698,BLAKE,MANAGER,7839,1981-05-01,2850.00,,30) | 7698 |
|
(7782,CLARK,MANAGER,7839,1981-06-09,2450.00,,10) | 7782 |
|
(7788,SCOTT,ANALYST,7566,1987-04-19,3000.00,,20) | 7788 |
|
(7839,KING,PRESIDENT,,1981-11-17,5000.00,,10) | 7839 | 10
| ACCOUNTING
(7844,TURNER,SALESMAN,7698,1981-09-08,1500.00,0.00,30) | 7844 |
|
(7876,ADAMS,CLERK,7788,1987-05-23,1100.00,,20) | 7876 |
|
(7900,JAMES,CLERK,7698,1981-12-03,950.00,,30) | 7900 |
|
(7902,FORD,ANALYST,7566,1981-12-03,3000.00,,20) | 7902 |
|
(7934,MILLER,CLERK,7782,1982-01-23,1300.00,,10) | 7934 |
|
(14 rows)
Explain verbose output for the query with LIMIT clause is:
postgres=# explain verbose select e, e.empno, d.deptno, d.dname from f_emp
e left join f_dept d on e.deptno = d.deptno and e.sal > 3000 order by 2, 3
limit 10;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------------------------------------------------------------------------------------------------------------------------------------
---------------------------
Limit (cost=100.00..136.86 rows=10 width=236)
Output: e.*, e.empno, d.deptno, d.dname
-> Foreign Scan (cost=100.00..2304.10 rows=598 width=236)
Output: e.*, e.empno, d.deptno, d.dname
Relations: (public.f_emp e) LEFT JOIN (public.f_dept d)
Remote SQL: SELECT CASE WHEN r1.* IS NOT NULL THEN ROW(r1.empno,
r1.ename, r1.job, r1.mgr, r1.hiredate, r1.sal, r1.comm, r1.deptno) END,
r1.empno, r2
.deptno, r2.dname FROM (public.emp r1 LEFT JOIN public.dept r2 ON (((r1.sal
3000::numeric)) AND ((r1.deptno = r2.deptno)))) ORDER BY r1.empno ASC
NULLS LAST
, r2.deptno ASC NULLS LAST
(6 rows)
Further looking I found that here problem is because we converting wholerow
reference with ROW - and binding it with CASE clause.
So, in the above example reference to "r" is converted with
"CASE WHEN r1.* IS NOT NULL THEN ROW(r1.empno, r1.ename, r1.job, r1.mgr,
r1.hiredate, r1.sal, r1.comm, r1.deptno) END"
Here r1.* IS NOT NULL is behaving strange, it return TRUE only when all the
elements in the wholerow is NOT NULL.
Example with normal table (not postgres_fdw involded):
postgres=# select r, r.* is null as isnull, r.* is not null as isnotnull
from emp r;
r | isnull |
isnotnull
-----------------------------------------------------------+--------+-----------
(7369,SMITH,CLERK,7902,1980-12-17,800.00,,20) | f | f
(7499,ALLEN,SALESMAN,7698,1981-02-20,1600.00,300.00,30) | f | t
(7521,WARD,SALESMAN,7698,1981-02-22,1250.00,500.00,30) | f | t
(7566,JONES,MANAGER,7839,1981-04-02,2975.00,,20) | f | f
(7654,MARTIN,SALESMAN,7698,1981-09-28,1250.00,1400.00,30) | f | t
(7698,BLAKE,MANAGER,7839,1981-05-01,2850.00,,30) | f | f
(7782,CLARK,MANAGER,7839,1981-06-09,2450.00,,10) | f | f
(7788,SCOTT,ANALYST,7566,1987-04-19,3000.00,,20) | f | f
(7839,KING,PRESIDENT,,1981-11-17,5000.00,,10) | f | f
(7844,TURNER,SALESMAN,7698,1981-09-08,1500.00,0.00,30) | f | t
(7876,ADAMS,CLERK,7788,1987-05-23,1100.00,,20) | f | f
(7900,JAMES,CLERK,7698,1981-12-03,950.00,,30) | f | f
(7902,FORD,ANALYST,7566,1981-12-03,3000.00,,20) | f | f
(7934,MILLER,CLERK,7782,1982-01-23,1300.00,,10) | f | f
(14 rows)
Now I was under impression the IS NOT NULL should be always in inverse of
IS NULL, but clearly here its not the case with wholerow. But further
looking at
the document its saying different thing for wholerow:
https://www.postgresql.org/docs/9.5/static/functions-comparison.html
Note: If the expression is row-valued, then IS NULL is true when the row
expression
itself is null or when all the row's fields are null, while IS NOT NULL is
true
when the row expression itself is non-null and all the row's fields are
non-null.
Because of this behavior, IS NULL and IS NOT NULL do not always return
inverse
results for row-valued expressions, i.e., a row-valued expression that
contains
both NULL and non-null values will return false for both tests. This
definition
conforms to the SQL standard, and is a change from the inconsistent behavior
exhibited by PostgreSQL versions prior to 8.2.
And as above documentation clearly says that IS NULL and IS NOT NULL do not
always return inverse results for row-valued expressions. So need to change
the
deparse logic into postgres_fdw - how ? May be to use IS NULL rather then IS
NOT NULL?
Input/thought?
Regards
Rushabh Lathia
www.EnterpriseDB.com
Attachments:
fdw_setup.sqltext/x-sql; charset=US-ASCII; name=fdw_setup.sqlDownload
On 2016/06/21 16:27, Rushabh Lathia wrote:
Now I was under impression the IS NOT NULL should be always in inverse of
IS NULL, but clearly here its not the case with wholerow. But further
looking at
the document its saying different thing for wholerow:https://www.postgresql.org/docs/9.5/static/functions-comparison.html
Note: If the expression is row-valued, then IS NULL is true when the row
expression
itself is null or when all the row's fields are null, while IS NOT NULL is
true
when the row expression itself is non-null and all the row's fields are
non-null.
Because of this behavior, IS NULL and IS NOT NULL do not always return
inverse
results for row-valued expressions, i.e., a row-valued expression that
contains
both NULL and non-null values will return false for both tests. This
definition
conforms to the SQL standard, and is a change from the inconsistent behavior
exhibited by PostgreSQL versions prior to 8.2.And as above documentation clearly says that IS NULL and IS NOT NULL do not
always return inverse results for row-valued expressions. So need to change
the
deparse logic into postgres_fdw - how ? May be to use IS NULL rather then IS
NOT NULL?Input/thought?
Perhaps - NOT expr IS NULL? Like in the attached.
explain verbose select e, e.empno, d.deptno, d.dname from f_emp e left
join f_dept d on e.deptno = d.deptno and e.sal > 3000 order by 2, 3 limit 10;
QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
-------------
Limit (cost=100.00..136.86 rows=10 width=236)
Output: e.*, e.empno, d.deptno, d.dname
-> Foreign Scan (cost=100.00..2304.10 rows=598 width=236)
Output: e.*, e.empno, d.deptno, d.dname
Relations: (public.f_emp e) LEFT JOIN (public.f_dept d)
Remote SQL: SELECT CASE WHEN NOT r1.* IS NULL THEN ROW(r1.empno,
r1.ename, r1.job, r1.mgr, r1.hiredate, r1.sal, r1.comm, r1.deptno) END,
r1.empno, r2.deptno
, r2.dname FROM (public.emp r1 LEFT JOIN public.dept r2 ON (((r1.sal >
3000::numeric)) AND ((r1.deptno = r2.deptno)))) ORDER BY r1.empno ASC
NULLS LAST, r2.deptno AS
C NULLS LAST
(6 rows)
select e, e.empno, d.deptno, d.dname from f_emp e left join f_dept d on
e.deptno = d.deptno and e.sal > 3000 order by 2, 3 limit 10;
e | empno | deptno | dname
-----------------------------------------------------------+-------+--------+------------
(7369,SMITH,CLERK,7902,1980-12-17,800.00,,20) | 7369 | |
(7499,ALLEN,SALESMAN,7698,1981-02-20,1600.00,300.00,30) | 7499 | |
(7521,WARD,SALESMAN,7698,1981-02-22,1250.00,500.00,30) | 7521 | |
(7566,JONES,MANAGER,7839,1981-04-02,2975.00,,20) | 7566 | |
(7654,MARTIN,SALESMAN,7698,1981-09-28,1250.00,1400.00,30) | 7654 | |
(7698,BLAKE,MANAGER,7839,1981-05-01,2850.00,,30) | 7698 | |
(7782,CLARK,MANAGER,7839,1981-06-09,2450.00,,10) | 7782 | |
(7788,SCOTT,ANALYST,7566,1987-04-19,3000.00,,20) | 7788 | |
(7839,KING,PRESIDENT,,1981-11-17,5000.00,,10) | 7839 |
10 | ACCOUNTING
(7844,TURNER,SALESMAN,7698,1981-09-08,1500.00,0.00,30) | 7844 | |
(10 rows)
Thanks,
Amit
Attachments:
wholerowvar-deparse-bug-1.patchtext/x-diff; name=wholerowvar-deparse-bug-1.patchDownload+2-2
On Tue, Jun 21, 2016 at 4:36 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp
wrote:
On 2016/06/21 16:27, Rushabh Lathia wrote:
Now I was under impression the IS NOT NULL should be always in inverse of
IS NULL, but clearly here its not the case with wholerow. But further
looking at
the document its saying different thing for wholerow:https://www.postgresql.org/docs/9.5/static/functions-comparison.html
Note: If the expression is row-valued, then IS NULL is true when the row
expression
itself is null or when all the row's fields are null, while IS NOT NULLis
true
when the row expression itself is non-null and all the row's fields are
non-null.
Because of this behavior, IS NULL and IS NOT NULL do not always return
inverse
results for row-valued expressions, i.e., a row-valued expression that
contains
both NULL and non-null values will return false for both tests. This
definition
conforms to the SQL standard, and is a change from the inconsistentbehavior
exhibited by PostgreSQL versions prior to 8.2.
And as above documentation clearly says that IS NULL and IS NOT NULL do
not
always return inverse results for row-valued expressions. So need to
change
the
deparse logic into postgres_fdw - how ? May be to use IS NULL ratherthen IS
NOT NULL?
Input/thought?
Perhaps - NOT expr IS NULL? Like in the attached.
As the documentation describes row is NULL is going to return true when all
the columns in a row are NULL, even though row itself is not null. So, with
your patch a row (null, null, null) is going to be output as a NULL row. Is
that right?
In an outer join we have to differentiate between a row being null (because
there was no joining row on nullable side) and a non-null row with all
column values being null. If we cast the whole-row expression to a text
e.g. r.*::text and test the resultant value for nullness, it gives us what
we want. A null row casted to text is null and a row with all null values
casted to text is not null.
postgres=# select (null, null, null)::text is not null;
?column?
----------
t
(1 row)
postgres=# select null::record::text is not null;
?column?
----------
f
(1 row)
In find_coercion_pathway(), we resort to IO coercion for record::text
explicit coercion. This should always work the way we want as record_out is
a strict function and for non-null values it outputs at least the
parenthesis which will be treated as non-null text.
2253 /*
2254 * If we still haven't found a possibility, consider automatic
casting
2255 * using I/O functions. We allow assignment casts to string
types and
2256 * explicit casts from string types to be handled this way.
(The
2257 * CoerceViaIO mechanism is a lot more general than that, but
this is
2258 * all we want to allow in the absence of a pg_cast entry.) It
would
2259 * probably be better to insist on explicit casts in both
directions,
2260 * but this is a compromise to preserve something of the
pre-8.3
2261 * behavior that many types had implicit (yipes!) casts to
text.
2262 */
PFA the patch with the cast to text. This is probably uglier than expected,
but I don't know any better test to find nullness of a record, the way we
want here. The patch also includes the expected output changes in the
EXPLAIN output.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Attachments:
pg_null_wr.patchapplication/x-download; name=pg_null_wr.patchDownload+50-18
On 2016/06/21 20:42, Ashutosh Bapat wrote:
On Tue, Jun 21, 2016 at 4:36 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp <mailto:Langote_Amit_f8@lab.ntt.co.jp>>
wrote:
On 2016/06/21 16:27, Rushabh Lathia wrote:
Now I was under impression the IS NOT NULL should be always in
inverse of
IS NULL, but clearly here its not the case with wholerow. But further
looking at
the document its saying different thing for wholerow:https://www.postgresql.org/docs/9.5/static/functions-comparison.html
Note: If the expression is row-valued, then IS NULL is true when
the row
expression
itself is null or when all the row's fields are null, while IS NOTNULL is
true
when the row expression itself is non-null and all the row'sfields are
non-null.
Because of this behavior, IS NULL and IS NOT NULL do not always return
inverse
results for row-valued expressions, i.e., a row-valued expression that
contains
both NULL and non-null values will return false for both tests. This
definition
conforms to the SQL standard, and is a change from theinconsistent behavior
exhibited by PostgreSQL versions prior to 8.2.
And as above documentation clearly says that IS NULL and IS NOT
NULL do not
always return inverse results for row-valued expressions. So need
to change
the
deparse logic into postgres_fdw - how ? May be to use IS NULLrather then IS
NOT NULL?
Input/thought?
Perhaps - NOT expr IS NULL? Like in the attached.
As the documentation describes row is NULL is going to return true when
all the columns in a row are NULL, even though row itself is not null.
So, with your patch a row (null, null, null) is going to be output as a
NULL row. Is that right?
Yeah, I think so.
In an outer join we have to differentiate between a row being null
(because there was no joining row on nullable side) and a non-null row
with all column values being null. If we cast the whole-row expression
to a text e.g. r.*::text and test the resultant value for nullness, it
gives us what we want. A null row casted to text is null and a row with
all null values casted to text is not null.
postgres=# select (null, null, null)::text is not null;
?column?
----------
t
(1 row)postgres=# select null::record::text is not null;
?column?
----------
f
(1 row)In find_coercion_pathway(), we resort to IO coercion for record::text
explicit coercion. This should always work the way we want as record_out
is a strict function and for non-null values it outputs at least the
parenthesis which will be treated as non-null text.
2253 /*
2254 * If we still haven't found a possibility, consider
automatic casting
2255 * using I/O functions. We allow assignment casts to
string types and
2256 * explicit casts from string types to be handled this way.
(The
2257 * CoerceViaIO mechanism is a lot more general than that,
but this is
2258 * all we want to allow in the absence of a pg_cast entry.)
It would
2259 * probably be better to insist on explicit casts in both
directions,
2260 * but this is a compromise to preserve something of the
pre-8.3
2261 * behavior that many types had implicit (yipes!) casts to
text.
2262 */
PFA the patch with the cast to text. This is probably uglier than
expected, but I don't know any better test to find nullness of a record,
the way we want here. The patch also includes the expected output
changes in the EXPLAIN output.
How about using a system column eg, ctid, for the CASE WHEN conversion;
in Rushabh's example the reference to "r1" would be converted with "CASE
WHEN r1.ctid IS NOT NULL THEN ROW(r1.empno, r1.ename, r1.job, r1.mgr,
r1.hiredate, r1.sal, r1.comm, r1.deptno) END". IMO I think that that
would be much simpler than Ashutosh's approach.
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
How about using a system column eg, ctid, for the CASE WHEN conversion; in
Rushabh's example the reference to "r1" would be converted with "CASE WHEN
r1.ctid IS NOT NULL THEN ROW(r1.empno, r1.ename, r1.job, r1.mgr,
r1.hiredate, r1.sal, r1.comm, r1.deptno) END". IMO I think that that would
be much simpler than Ashutosh's approach.
A foreign table can have a view, a regular table, another foreign table or
a materialised view a its target. A view does not support any of the system
columns, so none of them are available.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/21 21:37, Ashutosh Bapat wrote:
How about using a system column eg, ctid, for the CASE WHEN
conversion; in Rushabh's example the reference to "r1" would be
converted with "CASE WHEN r1.ctid IS NOT NULL THEN ROW(r1.empno,
r1.ename, r1.job, r1.mgr, r1.hiredate, r1.sal, r1.comm, r1.deptno)
END". IMO I think that that would be much simpler than Ashutosh's
approach.
A foreign table can have a view, a regular table, another foreign table
or a materialised view a its target. A view does not support any of the
system columns, so none of them are available.
You are right. Sorry for the noise.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016/06/21 20:42, Ashutosh Bapat wrote:
On Tue, Jun 21, 2016 at 4:36 PM, Amit Langote wrote:
On 2016/06/21 16:27, Rushabh Lathia wrote:
And as above documentation clearly says that IS NULL and IS NOT NULL do
not
always return inverse results for row-valued expressions. So need to
change
the
deparse logic into postgres_fdw - how ? May be to use IS NULL ratherthen IS
NOT NULL?
Input/thought?
Perhaps - NOT expr IS NULL? Like in the attached.
As the documentation describes row is NULL is going to return true when all
the columns in a row are NULL, even though row itself is not null. So, with
your patch a row (null, null, null) is going to be output as a NULL row. Is
that right?
Right.
In an outer join we have to differentiate between a row being null (because
there was no joining row on nullable side) and a non-null row with all
column values being null. If we cast the whole-row expression to a text
e.g. r.*::text and test the resultant value for nullness, it gives us what
we want. A null row casted to text is null and a row with all null values
casted to text is not null.
You are right. There may be non-null rows with all columns null which are
handled wrongly (as Rushabh reports) and the hack I proposed is not right
for. Especially if from non-nullable side as in the reported case, NULL
test for such a whole-row-var would produce the wrong result. Casting to
text as your patch does produces the correct behavior.
I wonder whether such a whole-row-var would arise from the nullable side
of a join? I guess not. Not that I'm saying we shouldn't account for that
case at all since any and every whole-row-var in the targetlist currently
gets that treatment, even those that are known non-nullable. Couldn't we
have prevented the latter somehow? IOW, only generate the CASE WHEN when
a Var being deparsed is known nullable as the comment there says:
deparse.c:
1639 /*
1640 * In case the whole-row reference is under an outer join then it has
1641 * to go NULL whenver the rest of the row goes NULL. Deparsing a join
1642 * query would always involve multiple relations, thus qualify_col
1643 * would be true.
1644 */
1645 if (qualify_col)
1646 {
1647 appendStringInfoString(buf, "CASE WHEN");
1648 ADD_REL_QUALIFIER(buf, varno);
1649 appendStringInfo(buf, "* IS NOT NULL THEN ");
1650 }
But I guess just fixing the expression as your patch does may be just fine.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016/06/22 17:11, Amit Langote wrote:
I wonder whether such a whole-row-var would arise from the nullable side
of a join? I guess not. Not that I'm saying we shouldn't account for that
case at all since any and every whole-row-var in the targetlist currently
gets that treatment, even those that are known non-nullable. Couldn't we
have prevented the latter somehow? IOW, only generate the CASE WHEN when
a Var being deparsed is known nullable as the comment there says:deparse.c:
1639 /*
1640 * In case the whole-row reference is under an outer join then it has
1641 * to go NULL whenver the rest of the row goes NULL. Deparsing a join
1642 * query would always involve multiple relations, thus qualify_col
1643 * would be true.
1644 */
1645 if (qualify_col)
1646 {
1647 appendStringInfoString(buf, "CASE WHEN");
1648 ADD_REL_QUALIFIER(buf, varno);
1649 appendStringInfo(buf, "* IS NOT NULL THEN ");
1650 }
I think we could address this in another way once we support deparsing
subqueries; rewrite the remote query into something that wouldn't need
the CASE WHEN conversion. For example, we currently have:
postgres=# explain verbose select ft2 from ft1 left join ft2 on ft1.a =
ft2.a;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------
Foreign Scan (cost=100.00..110.04 rows=1 width=32)
Output: ft2.*
Relations: (public.ft1) LEFT JOIN (public.ft2)
Remote SQL: SELECT CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a, r2.b)
END FROM (public.t1 r1 LEFT JOIN public.t2 r2 ON (((r1.a = r2.a))))
(4 rows)
However, if we support deparsing subqueries, the remote query in the
above example could be rewritten into something like this:
SELECT ss.c2 FROM t1 LEFT JOIN (SELECT t2.a, ROW(a, b) FROM t2) ss(c1,
c2) ON (t1.a = ss.c1);
So we would no longer need "CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a,
r2.b) END" in the target list in the remote query.
For the CASE WHEN conversion for a system column other than ctid, we
could also address this by replacing the whole-row reference in the IS
NOT NULL condition in that conversion with the system column reference.
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
I wonder whether such a whole-row-var would arise from the nullable side
of a join? I guess not. Not that I'm saying we shouldn't account for that
case at all since any and every whole-row-var in the targetlist currently
gets that treatment, even those that are known non-nullable. Couldn't we
have prevented the latter somehow? IOW, only generate the CASE WHEN when
a Var being deparsed is known nullable as the comment there says:deparse.c:
1639 /*
1640 * In case the whole-row reference is under an outer join then it has
1641 * to go NULL whenver the rest of the row goes NULL. Deparsing a join
1642 * query would always involve multiple relations, thus qualify_col
1643 * would be true.
1644 */
1645 if (qualify_col)
1646 {
1647 appendStringInfoString(buf, "CASE WHEN");
1648 ADD_REL_QUALIFIER(buf, varno);
1649 appendStringInfo(buf, "* IS NOT NULL THEN ");
1650 }But I guess just fixing the expression as your patch does may be just fine.
I thought about that, but it means that we have compute the nullable relids
(which isn't a big deal I guess). That's something more than necessary for
fixing the bug, which is the focus in beta stage right now.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On Wed, Jun 22, 2016 at 2:26 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:
On 2016/06/22 17:11, Amit Langote wrote:
I wonder whether such a whole-row-var would arise from the nullable side
of a join? I guess not. Not that I'm saying we shouldn't account for that
case at all since any and every whole-row-var in the targetlist currently
gets that treatment, even those that are known non-nullable. Couldn't we
have prevented the latter somehow? IOW, only generate the CASE WHEN when
a Var being deparsed is known nullable as the comment there says:deparse.c:
1639 /*
1640 * In case the whole-row reference is under an outer join then it has
1641 * to go NULL whenver the rest of the row goes NULL. Deparsing a join
1642 * query would always involve multiple relations, thus qualify_col
1643 * would be true.
1644 */
1645 if (qualify_col)
1646 {
1647 appendStringInfoString(buf, "CASE WHEN");
1648 ADD_REL_QUALIFIER(buf, varno);
1649 appendStringInfo(buf, "* IS NOT NULL THEN ");
1650 }I think we could address this in another way once we support deparsing
subqueries; rewrite the remote query into something that wouldn't need the
CASE WHEN conversion. For example, we currently have:postgres=# explain verbose select ft2 from ft1 left join ft2 on ft1.a =
ft2.a;
QUERY PLAN------------------------------------------------------------------------------------------------------------------------------------------
Foreign Scan (cost=100.00..110.04 rows=1 width=32)
Output: ft2.*
Relations: (public.ft1) LEFT JOIN (public.ft2)
Remote SQL: SELECT CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a, r2.b) END
FROM (public.t1 r1 LEFT JOIN public.t2 r2 ON (((r1.a = r2.a))))
(4 rows)However, if we support deparsing subqueries, the remote query in the above
example could be rewritten into something like this:SELECT ss.c2 FROM t1 LEFT JOIN (SELECT t2.a, ROW(a, b) FROM t2) ss(c1, c2)
ON (t1.a = ss.c1);So we would no longer need "CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a,
r2.b) END" in the target list in the remote query.
Right. Although, it means that the query processor at the other end has to
do extra work for pulling up the subqueries.
For the CASE WHEN conversion for a system column other than ctid, we could
also address this by replacing the whole-row reference in the IS NOT NULL
condition in that conversion with the system column reference.That would not work again as the system column reference would make sense
locally but may not be available at the foreign server e.g. foreign table
targeting a view a tableoid is requested.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/22 18:14, Ashutosh Bapat wrote:
I wonder whether such a whole-row-var would arise from the nullable side
of a join? I guess not. Not that I'm saying we shouldn't account for that
case at all since any and every whole-row-var in the targetlist currently
gets that treatment, even those that are known non-nullable. Couldn't we
have prevented the latter somehow? IOW, only generate the CASE WHEN when
a Var being deparsed is known nullable as the comment there says:deparse.c:
1639 /*
1640 * In case the whole-row reference is under an outer join then it has
1641 * to go NULL whenver the rest of the row goes NULL. Deparsing a join
1642 * query would always involve multiple relations, thus qualify_col
1643 * would be true.
1644 */
1645 if (qualify_col)
1646 {
1647 appendStringInfoString(buf, "CASE WHEN");
1648 ADD_REL_QUALIFIER(buf, varno);
1649 appendStringInfo(buf, "* IS NOT NULL THEN ");
1650 }But I guess just fixing the expression as your patch does may be just fine.
I thought about that, but it means that we have compute the nullable relids
(which isn't a big deal I guess). That's something more than necessary for
fixing the bug, which is the focus in beta stage right now.
Agreed.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016/06/22 18:16, Ashutosh Bapat wrote:
On Wed, Jun 22, 2016 at 2:26 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
I think we could address this in another way once we support
deparsing subqueries; rewrite the remote query into something that
wouldn't need the CASE WHEN conversion. For example, we currently have:postgres=# explain verbose select ft2 from ft1 left join ft2 on
ft1.a = ft2.a;QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------
Foreign Scan (cost=100.00..110.04 rows=1 width=32)
Output: ft2.*
Relations: (public.ft1) LEFT JOIN (public.ft2)
Remote SQL: SELECT CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a,
r2.b) END FROM (public.t1 r1 LEFT JOIN public.t2 r2 ON (((r1.a =
r2.a))))
(4 rows)However, if we support deparsing subqueries, the remote query in the
above example could be rewritten into something like this:SELECT ss.c2 FROM t1 LEFT JOIN (SELECT t2.a, ROW(a, b) FROM t2)
ss(c1, c2) ON (t1.a = ss.c1);So we would no longer need "CASE WHEN r2.* IS NOT NULL THEN
ROW(r2.a, r2.b) END" in the target list in the remote query.
Right. Although, it means that the query processor at the other end has
to do extra work for pulling up the subqueries.
Yeah, that's right. But this approach seems not so ugly...
For the CASE WHEN conversion for a system column other than ctid, we
could also address this by replacing the whole-row reference in the
IS NOT NULL condition in that conversion with the system column
reference.
That would not work again as the system column reference would make
sense locally but may not be available at the foreign server e.g.
foreign table targeting a view a tableoid is requested.
Maybe I'm confused, but I think that in the system-column case it's the
user's responsibility to specify system columns for foreign tables in a
local query only when that makes sense on the remote end, as shown in
the below counter example:
postgres=# create foreign table fv1 (a int, b int) server myserver
options (table_name 'v1');
CREATE FOREIGN TABLE
postgres=# select ctid, * from fv1;
ERROR: column "ctid" does not exist
CONTEXT: Remote SQL command: SELECT a, b, ctid FROM public.v1
where v1 is a view created on the remote server "myserver".
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 22, 2016 at 3:57 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:
On 2016/06/22 18:16, Ashutosh Bapat wrote:
On Wed, Jun 22, 2016 at 2:26 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:I think we could address this in another way once we support
deparsing subqueries; rewrite the remote query into something that
wouldn't need the CASE WHEN conversion. For example, we currently
have:postgres=# explain verbose select ft2 from ft1 left join ft2 on
ft1.a = ft2.a;QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------
Foreign Scan (cost=100.00..110.04 rows=1 width=32)
Output: ft2.*
Relations: (public.ft1) LEFT JOIN (public.ft2)
Remote SQL: SELECT CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a,
r2.b) END FROM (public.t1 r1 LEFT JOIN public.t2 r2 ON (((r1.a =
r2.a))))
(4 rows)However, if we support deparsing subqueries, the remote query in the
above example could be rewritten into something like this:SELECT ss.c2 FROM t1 LEFT JOIN (SELECT t2.a, ROW(a, b) FROM t2)
ss(c1, c2) ON (t1.a = ss.c1);So we would no longer need "CASE WHEN r2.* IS NOT NULL THEN
ROW(r2.a, r2.b) END" in the target list in the remote query.Right. Although, it means that the query processor at the other end has
to do extra work for pulling up the subqueries.
Yeah, that's right. But this approach seems not so ugly...
For the CASE WHEN conversion for a system column other than ctid, we
could also address this by replacing the whole-row reference in the
IS NOT NULL condition in that conversion with the system column
reference.That would not work again as the system column reference would make
sense locally but may not be available at the foreign server e.g.
foreign table targeting a view a tableoid is requested.Maybe I'm confused, but I think that in the system-column case it's the
user's responsibility to specify system columns for foreign tables in a
local query only when that makes sense on the remote end, as shown in the
below counter example:postgres=# create foreign table fv1 (a int, b int) server myserver options
(table_name 'v1');
CREATE FOREIGN TABLE
postgres=# select ctid, * from fv1;
ERROR: column "ctid" does not exist
CONTEXT: Remote SQL command: SELECT a, b, ctid FROM public.v1
But a ctid, when available, would rightly get nullified when referenced as
a column of table. What happens with the other system columns is we 0 them
out locally, whether they are available at the foreign server or not. We
never try to check whether they are available at the foreign server or not.
If we use such column's column name to decide whether to report 0 or null
and if that column is not available at the foreign table, we will get
error. I think we should avoid that. Those column anyway do not make any
sense.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/22 19:37, Ashutosh Bapat wrote:
On Wed, Jun 22, 2016 at 3:57 PM, Etsuro Fujita
Maybe I'm confused, but I think that in the system-column case it's
the user's responsibility to specify system columns for foreign
tables in a local query only when that makes sense on the remote
end, as shown in the below counter example:postgres=# create foreign table fv1 (a int, b int) server myserver
options (table_name 'v1');
CREATE FOREIGN TABLE
postgres=# select ctid, * from fv1;
ERROR: column "ctid" does not exist
CONTEXT: Remote SQL command: SELECT a, b, ctid FROM public.v1
But a ctid, when available, would rightly get nullified when referenced
as a column of table.
Really?
What happens with the other system columns is we 0
them out locally, whether they are available at the foreign server or
not. We never try to check whether they are available at the foreign
server or not. If we use such column's column name to decide whether to
report 0 or null and if that column is not available at the foreign
table, we will get error. I think we should avoid that. Those column
anyway do not make any sense.
Agreed except for oid. I think we should handle oid in the same way as
ctid, which I'll work on in the next CF.
I think the proposed idea of applying record::text explicit coercion to
a whole-row reference in the IS NOT NULL condition in the CASE WHEN
conversion would work as expected as you explained, but I'm concerned
that the cost wouldn't be negligible when the foreign table has a lot of
columns.
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
I think the proposed idea of applying record::text explicit coercion to a
whole-row reference in the IS NOT NULL condition in the CASE WHEN
conversion would work as expected as you explained, but I'm concerned that
the cost wouldn't be negligible when the foreign table has a lot of columns.
That's right, if the foreign server doesn't optimize the case for IS NOT
NULL, which it doesn't :)
I am happy to use any cheaper means e.g a function which counts number of
columns in a record. All we need here is a way to correctly identify when a
record is null and not null in the way we want (as described upthread). I
didn't find any quickly. Do you have any suggestions?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/24 15:44, Ashutosh Bapat wrote:
I think the proposed idea of applying record::text explicit coercion to a
whole-row reference in the IS NOT NULL condition in the CASE WHEN
conversion would work as expected as you explained, but I'm concerned that
the cost wouldn't be negligible when the foreign table has a lot of columns.That's right, if the foreign server doesn't optimize the case for IS NOT
NULL, which it doesn't :)I am happy to use any cheaper means e.g a function which counts number of
columns in a record. All we need here is a way to correctly identify when a
record is null and not null in the way we want (as described upthread). I
didn't find any quickly. Do you have any suggestions?
I'm now starting to wonder if it would be outright wrong to just use the
alias names of corresponding foreign tables directly for whole-row
references? So, instead of these in target lists of remote queries:
SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN ROW (r1.*) END, ...
Just:
SELECT r1, ...
It seems to produce the correct result. Although, I may be missing
something because CASE WHEN solution seems to me to be deliberately chosen.
In any case, attached patch doing the above did not change the results of
related regression tests (plans obviously did change since they don't
output the CASE WHENs in target lists anymore).
Also see the example below:
create extension postgres_fdw;
create server myserver foreign data wrapper postgres_fdw options (dbname
'postgres', use_remote_estimate 'true');
create user mapping for CURRENT_USER server myserver;
create table t1(a int, b int);
create table t2(a int, b int);
create foreign table ft1(a int, b int) server myserver options (table_name
't1');
create foreign table ft2(a int, b int) server myserver options (table_name
't2');
insert into t1 values (1), (2);
insert into t1 values (null, null);
insert into t2 values (1);
insert into t2 values (1, 2);
explain (costs off, verbose) select t1, t1 is null, t2, t2 is null from
ft1 t1 left join ft2 t2 on (t1.a = t2.a);
QUERY PLAN
---------------------------------------------------------------------------------------------
Foreign Scan
Output: t1.*, (t1.* IS NULL), t2.*, (t2.* IS NULL)
Relations: (public.ft1 t1) LEFT JOIN (public.ft2 t2)
Remote SQL: SELECT r1, r2 FROM (public.t1 r1 LEFT JOIN public.t2 r2 ON
(((r1.a = r2.a))))
(4 rows)
select t1, t1 is null as t1null, t2, t2 is null as t2null from ft1 t1 left
join ft2 t2 on (t1.a = t2.a);
t1 | t1null | t2 | t2null
------+--------+-------+--------
(1,) | f | (1,) | f
(1,) | f | (1,2) | f
(2,) | f | | t
(,) | t | | t
(4 rows))
alter server myserver options (set use_remote_estimate 'false');
analyze;
explain (costs off, verbose) select t1, t1 is null, t2, t2 is null from
ft1 t1 left join ft2 t2 on (t1.a = t2.a);
QUERY PLAN
------------------------------------------------------
Merge Left Join
Output: t1.*, (t1.* IS NULL), t2.*, (t2.* IS NULL)
Merge Cond: (t1.a = t2.a)
-> Sort
Output: t1.*, t1.a
Sort Key: t1.a
-> Foreign Scan on public.ft1 t1
Output: t1.*, t1.a
Remote SQL: SELECT a, b FROM public.t1
-> Sort
Output: t2.*, t2.a
Sort Key: t2.a
-> Foreign Scan on public.ft2 t2
Output: t2.*, t2.a
Remote SQL: SELECT a, b FROM public.t2
(15 rows)
select t1, t1 is null as t1null, t2, t2 is null as t2null from ft1 t1 left
join ft2 t2 on (t1.a = t2.a);
t1 | t1null | t2 | t2null
------+--------+-------+--------
(1,) | f | (1,) | f
(1,) | f | (1,2) | f
(2,) | f | | t
(,) | t | | t
(4 rows)
And produces the correct result for Rushabh's case.
Thoughts?
Thanks,
Amit
Attachments:
whole-row-var-deparse-1.patchtext/x-diff; name=whole-row-var-deparse-1.patchDownload+26-58
On Fri, Jun 24, 2016 at 1:59 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp
wrote:
On 2016/06/24 15:44, Ashutosh Bapat wrote:
I think the proposed idea of applying record::text explicit coercion to
a
whole-row reference in the IS NOT NULL condition in the CASE WHEN
conversion would work as expected as you explained, but I'm concernedthat
the cost wouldn't be negligible when the foreign table has a lot of
columns.
That's right, if the foreign server doesn't optimize the case for IS NOT
NULL, which it doesn't :)I am happy to use any cheaper means e.g a function which counts number of
columns in a record. All we need here is a way to correctly identifywhen a
record is null and not null in the way we want (as described upthread). I
didn't find any quickly. Do you have any suggestions?I'm now starting to wonder if it would be outright wrong to just use the
alias names of corresponding foreign tables directly for whole-row
references? So, instead of these in target lists of remote queries:SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.*)* END, ...
This is wrong. The deparsed query looks like
SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.col1, r1.col2, ...)*
END,
The reason for this is that the foreign table definition may not match the
target table definition. This has been explained in the comments that you
have deleted in your patch. Am I missing something?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/24 17:38, Ashutosh Bapat wrote:
On Fri, Jun 24, 2016 at 1:59 PM, Amit Langote wrote:
I'm now starting to wonder if it would be outright wrong to just use the
alias names of corresponding foreign tables directly for whole-row
references? So, instead of these in target lists of remote queries:SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.*)* END, ...
This is wrong. The deparsed query looks like
SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN *ROW (r1.col1, r1.col2, ...)*
END,
Yeah, I had noticed that in explain outputs (should have written like
that). My point though is why we don't consider dropping the CASE WHEN
... END target list item solution in favor of simply using the alias name
for a whole-row reference without affecting the correctness behavior wrt
outer joins. Using CASE WHEN to emit the correct result has revealed its
downside (this thread) although a simple whole-row reference would just
work without any special consideration.
The reason for this is that the foreign table definition may not match the
target table definition. This has been explained in the comments that you
have deleted in your patch. Am I missing something?
What may go wrong if we requested r1 (an alias name) in target list of the
sent query instead of ROW(r1.col1, ...) for a whole-row reference in the
original query? Fear of wrong set of data arriving or in wrong order or
something like that? This part, I'm not quite sure about.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 22, 2016 at 4:11 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
In an outer join we have to differentiate between a row being null (because
there was no joining row on nullable side) and a non-null row with all
column values being null. If we cast the whole-row expression to a text
e.g. r.*::text and test the resultant value for nullness, it gives us what
we want. A null row casted to text is null and a row with all null values
casted to text is not null.You are right. There may be non-null rows with all columns null which are
handled wrongly (as Rushabh reports) and the hack I proposed is not right
for. Especially if from non-nullable side as in the reported case, NULL
test for such a whole-row-var would produce the wrong result. Casting to
text as your patch does produces the correct behavior.
I agree, but I think we'd better cast to pg_catalog.text instead, just
to be safe. Committed that way.
--
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
On Wed, Jun 22, 2016 at 5:16 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
However, if we support deparsing subqueries, the remote query in the above
example could be rewritten into something like this:SELECT ss.c2 FROM t1 LEFT JOIN (SELECT t2.a, ROW(a, b) FROM t2) ss(c1, c2)
ON (t1.a = ss.c1);So we would no longer need "CASE WHEN r2.* IS NOT NULL THEN ROW(r2.a,
r2.b) END" in the target list in the remote query.Right. Although, it means that the query processor at the other end has to
do extra work for pulling up the subqueries.
I would be inclined to pick the method that generates cleaner SQL. I
doubt that difference in optimization speed matters here - it's
presumably very small, especially when compared to the cost of the
network round-trip.
--
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