More smarts about CoerceViaIO, and less stupidity about ArrayCoerceExpr

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: More smarts about CoerceViaIO, and less stupidity about ArrayCoerceExpr
Date: 2019-02-19 23:11:21
Message-ID: 27571.1550617881@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I poked into a recent complaint[1] about PG not being terribly smart
about whether an IS NOT NULL index predicate is implied by a WHERE
clause, and determined that there are a couple of places where we
are being less bright than we could be about CoerceViaIO semantics.
CoerceViaIO is strict independently of whether the I/O functions it
calls are (and they might not be --- in particular, domain_in isn't).
However, not everyplace knows that:

* clauses.c's contain_nonstrict_functions_walker() uses default logic
that will examine the referenced I/O functions to see if they're strict.
That's expensive, requiring several syscache lookups, and it might not
even give the right answer --- though fortunately it'd err in the
conservative direction.

* predtest.c's clause_is_strict_for() doesn't know anything about
CoerceViaIO, so it fails to make the proof requested in [1].

I started to fix this, and was in the midst of copying-and-pasting
contain_nonstrict_functions_walker's handling of ArrayCoerceExpr,
when I realized that that code is actually wrong:

return expression_tree_walker((Node *) ((ArrayCoerceExpr *) node)->arg,
contain_nonstrict_functions_walker,
context);

It should be recursing to itself, not to expression_tree_walker.
As coded, the strictness check doesn't get applied to the immediate
child node of the ArrayCoerceExpr, so that if that node is non-strict
we may arrive at the wrong conclusion.

contain_nonstrict_functions() isn't used in very many places, fortunately,
and ArrayCoerceExpr isn't that easy to produce either, which may explain
the lack of field reports. I was able to cons up this example though,
which demonstrates an incorrect conclusion about whether it's safe to
inline a function declared STRICT:

regression=# create table t1 (f1 int);
CREATE TABLE
regression=# insert into t1 values(1),(null);
INSERT 0 2
regression=# create or replace function sfunc(int) returns int[] language sql
as 'select array[0, $1]::bigint[]::int[]' strict;
CREATE FUNCTION
regression=# select sfunc(f1) from t1;
sfunc
----------
{0,1}
{0,NULL}
(2 rows)

Of course, since sfunc is strict, that last output should be NULL not
an array containing a NULL.

The attached patch fixes both of these things. At least the second
hunk needs to be back-patched. I'm less sure about whether the
CoerceViaIO changes merit back-patching; they're not fixing wrong
answers, but they are responding to a field complaint. Thoughts?

regards, tom lane

[1] https://www.postgresql.org/message-id/CAHkN8V9Rfh6uAjQLURJfnHsQfC_MYiFUSWEVcwVSiPdokmkniw%40mail.gmail.com

Attachment Content-Type Size
fix-planner-coercion-issues-1.patch text/x-diff 1.9 KB

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2019-02-19 23:28:14 Re: WAL insert delay settings
Previous Message Paul Ramsey 2019-02-19 23:09:39 Re: Compressed TOAST Slicing