Re: Postgres_fdw join pushdown - wrong results with whole-row reference

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