Thread: BUG #18399: Query plan optimization results in runtime error when hoisting cast from inside subquery

The following bug has been logged on the website:

Bug reference:      18399
Logged by:          Sawyer Knoblich
Email address:      scknoblich@gmail.com
PostgreSQL version: 16.2
Operating system:   Docker image on macOS
Description:

Hello, I have the following standalone query that operates on a dataset with
a text column and attempts to find all rows where that column's text is an
integer that is under a certain value:

```
with raw_data(id, value) as (values (1, '1900'), (2, '2100'), (3, 'abc')),
     integers as (select value::integer
                  from raw_data
                  where id in (select id from raw_data where value ~
'^\d+$'))
select count(*)
from integers
where value < 2000;
```

In this query the "integers" CTE attempts to find all rows where the "value"
column is able to be converted to an integer (using a subquery) and performs
that conversion, and then the main query does a simple where + count to
produce the required dataset. With this query my expectation was that the
main query would only be operating on values that had already been converted
to integers.

Running the query as-is results in `[22P02] ERROR: invalid input syntax for
type integer: "abc"`. From the query plan I can see that the optimizer has
chosen to directly replace `q.value` with `value::integer` in the outermost
where condition to be evaluated independently from the regex match, which
ends up attempting the "abc"::integer cast before its row is able to be
filtered out by the CTE's own condition:

```
Aggregate  (cost=0.20..0.21 rows=1 width=8)
  CTE raw_data
    ->  Values Scan on "*VALUES*"  (cost=0.00..0.04 rows=3 width=36)
  ->  Nested Loop Semi Join  (cost=0.00..0.16 rows=1 width=0)
        Join Filter: (raw_data.id = raw_data_1.id)
        ->  CTE Scan on raw_data  (cost=0.00..0.08 rows=1 width=4)
              Filter: ((value)::integer < 2000)
        ->  CTE Scan on raw_data raw_data_1  (cost=0.00..0.07 rows=1
width=4)
              Filter: (value ~ '^\d+$'::text)
```

I have also reproduced this behavior when using a different condition on the
main query such as a simple null check, when selecting from a larger
persisted table instead of the "raw_data" CTE used here for simplicity, and
when using a subquery instead of the "integers" CTE. I did find that if I
remove the subquery inside the CTE's where clause and instead apply the
regex directly to the "value" column then I get the following query plan
which succeeds with no issues:

```
with raw_data(id, value) as (values (1, '1900'), (2, '2100'), (3, 'abc')),
     integers as (select value::integer
                  from raw_data
                  where value ~ '^\d+$')
select count(*)
from integers
where value < 2000;
----------
Aggregate  (cost=0.07..0.08 rows=1 width=8)
  ->  Values Scan on "*VALUES*"  (cost=0.00..0.07 rows=1 width=0)
        Filter: ((column2 ~ '^\d+$'::text) AND ((column2)::integer <
2000))
```

My expectation was that the query should behave as if the CTE (or subquery
if I had used that instead) has fully executed before moving on to the main
query and this behavior breaks that assumption (which may or may not be
correct). I attempted to search the docs for anything related to this,
either CTE/subquery execution order guarantees or hoisting of casts, but I
couldn't find anything about it. If this turns out not to a bug and is just
expected behavior with existing documentation could you please link me to
the relevant page for future reference?

PG Bug reporting form <noreply@postgresql.org> writes:
> My expectation was that the query should behave as if the CTE (or subquery
> if I had used that instead) has fully executed before moving on to the main
> query and this behavior breaks that assumption (which may or may not be
> correct). I attempted to search the docs for anything related to this,
> either CTE/subquery execution order guarantees or hoisting of casts, but I
> couldn't find anything about it. If this turns out not to a bug and is just
> expected behavior with existing documentation could you please link me to
> the relevant page for future reference?

You need to materialize the CTE to be certain that it won't be
intermingled with the calling query:

=# with raw_data(id, value) as (values (1, '1900'), (2, '2100'), (3, 'abc')),
     integers as materialized (select value::integer
                  from raw_data
                  where id in (select id from raw_data where value ~ '^\d+$'))
select count(*)
from integers
where value < 2000;
 count 
-------
     1
(1 row)

See
https://www.postgresql.org/docs/current/sql-select.html#SQL-WITH
(last few paras of that sub-section).

The real bottom line here is that the cast to integer is presumed
to be side-effect-free, which isn't true if you're working with
data where it could throw errors.  My advice is to rethink your
data model: this sort of EAV approach with inconsistent data
types in the "same" column just does not play nice with SQL
optimizers, so it'll cause you ongoing heartburn.  If you think
you're too far down the road for that, consider replacing the
cast with some non-error-throwing user-defined function,
about like

  case when x ~ '^\d+$' then x::integer else null::integer end

            regards, tom lane

Sawyer Knoblich <scknoblich@gmail.com> writes:
> To make sure I'm understanding correctly, are you suggesting that this is
> not a bug? The mention of "the cast to integer is presumed to be
> side-effect-free, which isn't true if you're working with data where it
> could throw errors" to me heavily implies that this is a faulty assumption
> on behalf of the optimizer about what behaviors certain parts of the query
> may have, but I didn't get the impression that this is being considered an
> optimizer bug and is instead something that just needs to be worked around
> or avoided (although if I'm not interpreting it right then definitely
> please correct me).

It is not an optimizer bug: the optimizer is doing what the function
marking entitles it to.  You could argue that it's a marking bug and
no function/operator that can throw an error should be marked stable
or immutable, because throwing an error is a kind of side-effect.
But that would be a pretty useless answer, as it would nearly destroy
our ability to optimize queries at all, since there's not all that
many functions that can be guaranteed not to throw errors.

In short, this state of affairs is an engineering compromise.
It's not perfect, but it's not easy to see how to do better either.

            regards, tom lane

On Wed, 20 Mar 2024 at 16:10, Sawyer Knoblich <scknoblich@gmail.com> wrote:
> In short, it felt to me like fallibility was orthogonal to volatility in that its side effect is only triggered by
callingit the first time instead of by calling it multiple times. My thought was of a potential second type of function
markerto describe fallibility that the optimizer could possibly leverage to hopefully still enable most types of
optimizationswhile preventing scenarios such as this one. I was imagining that an operation like division could be
markedsomething like "immutable fallible", which would indicate that the function is still allowed to be optimized in
anyway that an immutable function is except for any optimizations that may cause it to operate on additional input that
itwould otherwise not normally operate on in an unoptimized query, which could then be used to disallow this bug's
optimizationwhere the values given to the cast are first filtered by the where clause. 

Thanks for thinking about it, but I don't think this would work as you
only have to have > 1 condition that could cause ERRORs for the
optimiser not to know the "correct" order of evaluation.   Aside from
that, just because the evaluation of the expression could fail, it
does not mean that it *will* fail with the given data.  For example,
the addition of two BIGINT values *could* fail due to overflow, but
it's unlikely that it will fail with most numbers that fit into that
type. So restricting the order of evaluation for conditions which
could fail is likely to upset more people than it will please.

There are quite a lot of things which would have to be restricted,
much more than you might think.  Any conditions which could cause an
error would have to be evaluated last in a WHERE clause and that might
result in being unable to use indexes because some other (possibly
unindexed) expression would need to be evaluated first.

As for documentation, I wonder if it's worth a paragraph in [1] to
mention SQL is a declarative language and mention a few caveats that
could come with that which might catch out people who are used to
procedural languages.

David

[1] https://www.postgresql.org/docs/current/queries-overview.html

David Rowley <dgrowleyml@gmail.com> writes:
> There are quite a lot of things which would have to be restricted,
> much more than you might think.  Any conditions which could cause an
> error would have to be evaluated last in a WHERE clause and that might
> result in being unable to use indexes because some other (possibly
> unindexed) expression would need to be evaluated first.

That particular aspect might not be too awful, because there's already
a good deal of pressure for index opclass members to not fail ---
certainly I'd expect that all standard btree comparison functions
could be marked non-failing.  (Let us slide quietly past the question
of exactly how strong the guarantee should be; for example any function
that takes toastable argument types is potentially at risk of OOM,
but do you really want to exclude numeric_eq and texteq from the
set of safe operations?  See also past discussions on how strict
we should be about the related concept of leakproofness.)

In any case I agree with your larger point that ordering things such
that potentially-failing tests are always done last would be a
performance disaster, even if it's possible at all.

            regards, tom lane