Re: Postgres_fdw join pushdown - wrong results with whole-row reference - Mailing list pgsql-hackers

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