Обсуждение: 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:
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?
Rushabh Lathia
Вложения
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
Вложения
On Tue, Jun 21, 2016 at 4:36 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
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.
Perhaps - NOT expr IS NULL? Like in the attached.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?
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 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 */
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
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Вложения
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 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. > 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
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
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
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 rather
>> 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?
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
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
------------------------------------------------------------------------------------------------------------------------------------------
ForeignScan (cost=100.00..110.04 rows=1 width=32) Output: ft2.* Relations: (public.ft1) LEFT JOIN (public.ft2)
RemoteSQL: 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
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
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:
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.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.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
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
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
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
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
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
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
Вложения
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 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, ...
This is wrong. The deparsed query looks like
SELECT CASE WHEN (r1.*)::text IS NOT NULL THEN ROW (r1.col1, r1.col2, ...) END,
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
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
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
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
On Sat, Jun 25, 2016 at 12:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:
-- 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.
postgres_fdw resets the search path to pg_catalog while opening connection to the server. The reason behind this is explained in deparse.c
* We assume that the remote session's search_path is exactly "pg_catalog",
* and thus we need schema-qualify all and only names outside pg_catalog.
* We assume that the remote session's search_path is exactly "pg_catalog",
* and thus we need schema-qualify all and only names outside pg_catalog.
So, we should not be schema-qualifying types while casting. From deparse.c
/*
* Convert type OID + typmod info into a type name we can ship to the remote
* server. Someplace else had better have verified that this type name is
* expected to be known on the remote end.
*
* This is almost just format_type_with_typemod(), except that if left to its
* own devices, that function will make schema-qualification decisions based
* on the local search_path, which is wrong. We must schema-qualify all
* type names that are not in pg_catalog. We assume here that built-in types
* are all in pg_catalog and need not be qualified; otherwise, qualify.
*/
static char *
deparse_type_name(Oid type_oid, int32 typemod)
{
if (is_builtin(type_oid))
return format_type_with_typemod(type_oid, typemod);
else
return format_type_with_typemod_qualified(type_oid, typemod);
}
/*
* Convert type OID + typmod info into a type name we can ship to the remote
* server. Someplace else had better have verified that this type name is
* expected to be known on the remote end.
*
* This is almost just format_type_with_typemod(), except that if left to its
* own devices, that function will make schema-qualification decisions based
* on the local search_path, which is wrong. We must schema-qualify all
* type names that are not in pg_catalog. We assume here that built-in types
* are all in pg_catalog and need not be qualified; otherwise, qualify.
*/
static char *
deparse_type_name(Oid type_oid, int32 typemod)
{
if (is_builtin(type_oid))
return format_type_with_typemod(type_oid, typemod);
else
return format_type_with_typemod_qualified(type_oid, typemod);
}
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/25 4:14, Robert Haas wrote: > Committed that way. Thanks for taking care of this! I found another bug in error handling of whole-row references in join pushdown; conversion_error_callback fails to take into account that get_relid_attribute_name(Oid relid, AttrNumber attnum) can't handle whole-row references (ie, attnum=0), in which case that would cause cache lookup errors. Attached is a small patch to address this issue. Best regards, Etsuro Fujita
Вложения
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Mon, Jun 27, 2016 at 3:06 PM, Etsuro Fujita<span dir="ltr"><<a href="mailto:fujita.etsuro@lab.ntt.co.jp" target="_blank">fujita.etsuro@lab.ntt.co.jp</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex">On 2016/06/25 4:14, Robert Haas wrote:<br /><blockquote class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Committed that way.<br /></blockquote><br/> Thanks for taking care of this!<br /><br /> I found another bug in error handling of whole-row referencesin join pushdown; conversion_error_callback fails to take into account that get_relid_attribute_name(Oid relid,AttrNumber attnum) can't handle whole-row references (ie, attnum=0), in which case that would cause cache lookup errors. Attached is a small patch to address this issue.<br /></blockquote></div><br /></div><div class="gmail_extra">Doyou have any testcase reproducing the bug here? It would be good to include that test in the regression.<br/><br /></div><div class="gmail_extra">There is a always a possibility that a user would create a table (whichcan be used as target for the foreign table) with column named 'wholerow', in which case s/he will get confused withthis error message.<br /><br /></div><div class="gmail_extra">-- <br /><div class="gmail_signature" data-smartmail="gmail_signature"><divdir="ltr">Best Wishes,<br />Ashutosh Bapat<br />EnterpriseDB Corporation<br />The PostgresDatabase Company<br /></div></div></div></div>
On Mon, Jun 27, 2016 at 2:47 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Sat, Jun 25, 2016 at 12:44 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> 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. > > postgres_fdw resets the search path to pg_catalog while opening connection > to the server. The reason behind this is explained in deparse.c > > * We assume that the remote session's search_path is exactly "pg_catalog", > * and thus we need schema-qualify all and only names outside pg_catalog. Hmm. OK, should we revert the schema-qualification part of that commit, or just leave it alone? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2016/06/27 18:56, Ashutosh Bapat wrote: > On Mon, Jun 27, 2016 at 3:06 PM, Etsuro Fujita > <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote: > I found another bug in error handling of whole-row references in > join pushdown; conversion_error_callback fails to take into account > that get_relid_attribute_name(Oid relid, AttrNumber attnum) can't > handle whole-row references (ie, attnum=0), in which case that would > cause cache lookup errors. Attached is a small patch to address > this issue. > Do you have any testcase reproducing the bug here? It would be good to > include that test in the regression. Done. > There is a always a possibility that a user would create a table (which > can be used as target for the foreign table) with column named > 'wholerow', in which case s/he will get confused with this error message. By grepping I found that there are error messages that use "whole-row table reference", "whole-row reference", or "wholerow", so the use of "wholerow" seems to me reasonable. (And IMO I think "wholerow" would most likely fit into that errcontext().) Best regards, Etsuro Fujita
Вложения
On Tue, Jun 28, 2016 at 9:00 AM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
On 2016/06/27 18:56, Ashutosh Bapat wrote:On Mon, Jun 27, 2016 at 3:06 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:I found another bug in error handling of whole-row references in
join pushdown; conversion_error_callback fails to take into account
that get_relid_attribute_name(Oid relid, AttrNumber attnum) can't
handle whole-row references (ie, attnum=0), in which case that would
cause cache lookup errors. Attached is a small patch to address
this issue.Do you have any testcase reproducing the bug here? It would be good to
include that test in the regression.
Done.There is a always a possibility that a user would create a table (which
can be used as target for the foreign table) with column named
'wholerow', in which case s/he will get confused with this error message.
By grepping I found that there are error messages that use "whole-row table reference", "whole-row reference",
These two should be fine.
or "wholerow", so the use of "wholerow" seems to me reasonable. (And IMO I think "wholerow" would most likely fit into that errcontext().)
As an error message text, which is not referring to any particular column, these are fine. But in this case, we are specifically referring to a particular column. That reference can be confusing. The text ' column "wholerow" of foreign table "ft1"' is confusing either way. A lay user who doesn't have created table with "wholerow" column, s/he will not understand which column the error context refers to. For a user who has created table with "wholerow" column, he won't find any problem with the data in that column.
Ideally, we should point out the specific column that faced the conversion problem and report it, instead of saying the whole row reference conversion caused the problem. But that may be too difficult. Or at least the error message should read "whole row reference of foreign table ft1".
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/28 13:53, Ashutosh Bapat wrote: > Ideally, we should point out the specific column that faced the > conversion problem and report it, instead of saying the whole row > reference conversion caused the problem. But that may be too difficult. I think so too. > Or at least the error message should read "whole row reference of > foreign table ft1". OK, attached is an updated version of the patch, which uses "whole-row reference", not "wholerow". Best regards, Etsuro Fujita
Вложения
On Tue, Jun 28, 2016 at 11:43 AM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
On 2016/06/28 13:53, Ashutosh Bapat wrote:Ideally, we should point out the specific column that faced the
conversion problem and report it, instead of saying the whole row
reference conversion caused the problem. But that may be too difficult.
I think so too.Or at least the error message should read "whole row reference of
foreign table ft1".
OK, attached is an updated version of the patch, which uses "whole-row reference", not "wholerow".
The wording "column "whole-row reference ..." doesn't look good. Whole-row reference is not a column. The error context itself should be "whole row reference for foreign table ft1".
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/06/28 15:23, Ashutosh Bapat wrote: > The wording "column "whole-row reference ..." doesn't look good. > Whole-row reference is not a column. The error context itself should be > "whole row reference for foreign table ft1". Ah, you are right. Please find attached an updated version. Best regards, Etsuro Fujita
Вложения
Hmm. OK, should we revert the schema-qualification part of that>
> postgres_fdw resets the search path to pg_catalog while opening connection
> to the server. The reason behind this is explained in deparse.c
>
> * We assume that the remote session's search_path is exactly "pg_catalog",
> * and thus we need schema-qualify all and only names outside pg_catalog.
commit, or just leave it alone?
If we leave that code as is, someone who wants to add similar code later would get confused or will be tempted to create more instances of schema-qualification. I think we should revert the schema qualification.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Tue, Jun 28, 2016 at 12:52 PM, Etsuro Fujita<span dir="ltr"><<a href="mailto:fujita.etsuro@lab.ntt.co.jp" target="_blank">fujita.etsuro@lab.ntt.co.jp</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 2016/06/28 15:23, Ashutosh Bapat wrote:<br /><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The wording "column"whole-row reference ..." doesn't look good.<br /> Whole-row reference is not a column. The error context itself shouldbe<br /> "whole row reference for foreign table ft1".<br /></blockquote><br /></span> Ah, you are right. Please findattached an updated version.<br /><br clear="all" /></blockquote></div><br /></div><div class="gmail_extra">This looksgood to me. Regression tests pass.<br /><br /></div><div class="gmail_extra">-- <br /><div class="gmail_signature" data-smartmail="gmail_signature"><divdir="ltr">Best Wishes,<br />Ashutosh Bapat<br />EnterpriseDB Corporation<br />The PostgresDatabase Company<br /></div></div></div></div>
On Tue, Jun 28, 2016 at 8:20 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: >> > postgres_fdw resets the search path to pg_catalog while opening >> > connection >> > to the server. The reason behind this is explained in deparse.c >> > >> > * We assume that the remote session's search_path is exactly >> > "pg_catalog", >> > * and thus we need schema-qualify all and only names outside >> > pg_catalog. >> >> Hmm. OK, should we revert the schema-qualification part of that >> commit, or just leave it alone? > > If we leave that code as is, someone who wants to add similar code later > would get confused or will be tempted to create more instances of > schema-qualification. I think we should revert the schema qualification. OK, done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Jul 1, 2016 at 7:45 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jun 28, 2016 at 8:20 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>> > postgres_fdw resets the search path to pg_catalog while opening
>> > connection
>> > to the server. The reason behind this is explained in deparse.c
>> >
>> > * We assume that the remote session's search_path is exactly
>> > "pg_catalog",
>> > * and thus we need schema-qualify all and only names outside
>> > pg_catalog.
>>
>> Hmm. OK, should we revert the schema-qualification part of that
>> commit, or just leave it alone?
>
> If we leave that code as is, someone who wants to add similar code later
> would get confused or will be tempted to create more instances of
> schema-qualification. I think we should revert the schema qualification.
OK, done.
Thanks.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On Wed, Jun 29, 2016 at 1:38 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Tue, Jun 28, 2016 at 12:52 PM, Etsuro Fujita > <fujita.etsuro@lab.ntt.co.jp> wrote: >> On 2016/06/28 15:23, Ashutosh Bapat wrote: >>> >>> The wording "column "whole-row reference ..." doesn't look good. >>> Whole-row reference is not a column. The error context itself should be >>> "whole row reference for foreign table ft1". >> >> Ah, you are right. Please find attached an updated version. > > This looks good to me. Regression tests pass. Committed. Thanks to Etsuro Fujita for the patch and to Ashutosh for the review. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2016/07/02 0:32, Robert Haas wrote: > On Wed, Jun 29, 2016 at 1:38 AM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> On Tue, Jun 28, 2016 at 12:52 PM, Etsuro Fujita >> <fujita.etsuro@lab.ntt.co.jp> wrote: >>> Please find attached an updated version. >> This looks good to me. Regression tests pass. > Committed. Thanks to Etsuro Fujita for the patch and to Ashutosh for > the review. Thanks, Robert and Ashutosh! Best regards, Etsuro Fujita