[YSQL] Wrong results from Index Scan where index is based on an expression with a user-defined operator

Jira Link: DB-2641

Important

Search for « I replaced the existing "issue-12897.zip" with a new version: HERE. This contains a significantly simplified testcase. » and read carefully from there on. You can ignore the account of the code in my original bug description. (But the code organization is unchanged.) Notice that the ultimate conclusion about the demonstration of a bug is unchanged. The new testcase simply removes a fair amount of extraneous clutter so that the analysis can be cleaner.

Original bug description

This is a wrong results issue: a select with a where clause that identifies just one row gets all rows.

The issue occurs in the presence of three conditions:

The where predicate uses an expression with a user-defined operator, +>>, in place of the native ->> operator on a jsonb column.
The where predicate is supported by an expression-based index on the expression with the user-defined operator.
The JSON table column, and all the other data types of interest, are user-defined domains that specify a not null constraint—for example, _textnn.

The +>> operator is defined and used as a correctness measure. When the native ->> operator mentions the name of an object key that is not present, it simply returns the SQL null. But the (informal) JSON Schema for the incoming documents in my particular use case specifies a small set of object keys where every one is required and cannot have the value JSON null. If the application code mis-spells a key name, then the SQL null that the native ->> operator returns will bring downstream problems.

The code kit

This is provided in the attached issue-12897.zip. Run the scripts in any sandbox environment, as explained below. (I used an empty schema owned by an ordinary user.)

The zip contains a master script, 0.sql, that invokes six worker scripts thus:

\t off
\ir set-up/0-cr-caption.sql
\ir set-up/1-create-domains.sql
\ir set-up/2-create-table.sql
\ir set-up/3-create-strict-extract-operator.sql
\ir set-up/4-populate-table.sql
\ir demo.sql

The 4-populate-table.sql script inserts ten easily searchable rows and then randomly-generated rows to bring the total to 100,000. This demo.sql script:

drop index if exists native_operator       cascade;
drop index if exists user_defined_operator cascade;

\t on
select caption('NO indexes');
\t off
\i qry.sql

create unique index native_operator       on t((doc->>'key')); 
create unique index user_defined_operator on t((doc+>>'key')); 

\t on
select caption('WITH indexes');
\t off
\i qry.sql

And this is qry.sql:

deallocate all;

set pg_hint_plan.enable_hint = on;

prepare qry as
with c(k, "->>'key'", "+>>'key'") as (
  select /*+ NoSeqScan(t) IndexScan(t) +*/
    k,
    doc->>'key',
    doc+>>'key'
  from t
  where doc+>>'key' = 'dog'::text_nn)
select
   k,
   "->>'key'",
   ("->>'key'" = "+>>'key'")::text_nn as same
from c order by k;

execute qry;
explain execute qry;

Look at 3-create-strict-extract-operator on the set-up child directory. It starts like this:

create function "strict +>>"(j in jsonb, key_name in text_nn)

    returns text_nn  -- As designed (provokes YB bug).
 -- returns text     -- Workaround  (no YB bug)

  immutable
  language plpgsql
as $body$

This is the critical point — and it implies that there are four possible test runs. It helps, therefore, to spool the output so that four outputs can be diff'ed. Two driver scripts are provided thus:

To be run using vanilla PG:

\o output/PG.txt
\t on
\i 0.sql
\t off
\o

To be run using YB:

\o output/YB.txt
\t on
\i 0.sql
\t off
\o

I re-named the output file after each test to reflect the conditions. I therefore ended up with these four files on the output directory:

PG-as-designed-0.txt
PG-workaround-0.txt
YB-as-designed-0.txt
YB-workaround-0.txt

I added the trailing "-0" suffixes to distinguish my results, provided in the .zip, from yours (presumably without the suffix).

Results

I've copied the critical lines from the four files here to make the comparison (shown easily by diff'ing) more readily discernible, thus:

There are just two distinct predicates: one for the "As designed" case; and one for the "Workaround" case, thus:

As designed: ((((doc)::jsonb +>> ('key'::text)::text_nn))::text = (('dog'::text)::text_nn)::text)

Workaround:  (((doc)::jsonb +>> ('key'::text)::text_nn) = (('dog'::text)::text_nn)::text)

In each case, the same predicate is used for both the Filter case and the Index Cond case with each of PG and YB. This seems to suggest that YB has faithfully re-captured PG's behavior. The typecasts are expected. Try _poc-for-user-defined-nndomains.sql to confirm this.

But look at the YB-as-designed output file. This, uniquely among the four output files, and only for the case where the index based on the user-defined operator is used, shows that the predicate has no effect and that all 100,000 rows were selected.

Using PG

Without index:

As designed: Filter: ((((doc)::jsonb +>> ('key'::text)::text_nn))::text = (('dog'::text)::text_nn)::text)

Workaround:  Filter: (((doc)::jsonb +>> ('key'::text)::text_nn) = (('dog'::text)::text_nn)::text)

With index (Index Scan using "t.doc+>>'key' unq" on t ):

As designed: Index Cond: ((((doc)::jsonb +>> ('key'::text)::text_nn))::text = (('dog'::text)::text_nn)::text)

Workaround:  Index Cond: (((doc)::jsonb +>> ('key'::text)::text_nn) = (('dog'::text)::text_nn)::text)

Using YB

Without index:

As designed: Filter: ((((doc)::jsonb +>> ('key'::text)::text_nn))::text = (('dog'::text)::text_nn)::text)

Workaround:  Filter: (((doc)::jsonb +>> ('key'::text)::text_nn) = (('dog'::text)::text_nn)::text)

With index (Index Scan using "t.doc+>>'key' unq" on t ):

As designed: Index Cond: ((((doc)::jsonb +>> ('key'::text)::text_nn))::text = (('dog'::text)::text_nn)::text)

Workaround:  Index Cond: (((doc)::jsonb +>> ('key'::text)::text_nn) = (('dog'::text)::text_nn)::text)

More background

I discussed this, and related JSON phenomena, on the pgsql-general email list. Tom Lane replied HERE thus:

[returning SQL null for a missing key] is hard to justify from a purist semantic point of view, but having the operator throw an error in such cases would make it close to unusable on not-uniformly-structured data. And really the point of using JSON inside a SQL database is to cope with irregularly-structured data, so fuzziness seems like what we want.

I'm convinced by Tom's "close to unusable" assessment for the general case. But I have uniformly-structured data and I do want the errors that are a nuisance in other use cases. It's easy to write a user-defined operator that gives me what I want under the circumstances under discussion. And it works fine in vanilla PostgreSQL. But it gets terrible wrong results in YB.

Bonus test

Now try the bonus.sql script. It attempts to take JSON out of the picture by implementing a user-defined operator—as a trivial wrapper to extract a fixed-length substring form a nominated text value starting at a nominated position, thus:

-- As designed (provokes YB bug).
create function "+>>"(string in text_nn, pos in int_nn)
  returns text_nn  
  immutable
  language sql
as $body$
  select substr(string, pos, 6)::text_nn;
$body$;

create operator +>> (
  leftarg   = text_nn,
  rightarg  = int_nn,
  procedure = "+>>");

In this case the YB bug comes sooner and is more dramatic., thus

create unique index i on t((v+>>4));

It fails with this error:

XX000: Aborted: ERROR: type "text_nn" does not exist

But if you do this immediately after the error:

select 'dog'::text_nn;

you'll see that _textnn most certainly does exsist.

@bllewell A file in the attached zip fails to extract on macOS (perhaps on Linux, too). Please rename and reattach.

[mtakahara@Hals-MBP 12897]$ unzip issue-12897.zip 
Archive:  issue-12897.zip
  inflating: 0.sql                   
  inflating: demo.sql                
   creating: output/
  inflating: output/.DS_Store        
  inflating: output/PG-as-designed-0.txt  
  inflating: output/PG-workaround-0.txt  
  inflating: output/YB-as-designed-0.txt  
  inflating: output/YB-workaround-0.txt  
 extracting: pg.sql                  
  inflating: poc-for-user-defined-nn_domains.sql  
  inflating: qry.sql                 
   creating: set-up/
  inflating: set-up/.DS_Store        
  inflating: set-up/0-cr-caption.sql  
  inflating: set-up/1-create-domains.sql  
  inflating: set-up/2-create-table.sql  
error:  cannot create set-up/3-create-strict+?????operator.sql
        Illegal byte sequence
  inflating: set-up/4-populate-table.sql  
 extracting: yb.sql

Ah, yes... I see what the problem is. I had used this filename:

3-create-strict+>>operator.sql

And why not? It's a good name! Except that it turns out not to be for the reason that you gave. Grr... So I changed the name to this:

3-create-strict-extract-operator.sql

and I made the corresponding change in the 0.sql master script.

I also moved the "bonus" code into the one-and-only new issue-12897.zip as the single file bonus.sql. And I changed the wording in my account (above) to reflect this.

I uploaded the new issue-12897.zip. And I removed the now-redundant issue-12897-bonus.zip.

Took a quick look. Actually, based on our past discussion with @bllewell and @m-iancu on indexing a user defined type column, I thought we didn't have a custom comparator support, so, I wasn't sure what the Index Cond: ((((doc)::jsonb +>> ('key'::text)::text_nn))::text = (('dog'::text)::text_nn)::text) actually means - we wouldn't be able to evaluate it.

The docdb request dump indeed shows that pggate is not sending the predicate at all. EXPLAIN ANALYZE doesn't show the "Recheck" either. The predicate is simply left out while creating the docdb request.

explain analyze select /*+ IndexScan(t) */
    k,
    doc->>'key',
    doc+>>'key'
  from t
  where doc+>>'key' = 'dog'::text_nn;

                                                        QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------
 Index Scan using user_defined_operator on t  (cost=0.00..4.63 rows=1 width=68) (actual time=6.239..6.329 rows=10 loops=1)
   Index Cond: ((((doc)::jsonb +>> ('key'::text)::text_nn))::text = (('dog'::text)::text_nn)::text)
 Planning Time: 0.214 ms
 Execution Time: 6.457 ms
(4 rows)

docdb req (showing the secondary index scan request only):

I0623 22:28:19.484846 354790912 pg_session.cc:237] Applying operation: { READ active: 1 read_time: { read: <invalid> local_limit: <invalid> global_limit: <invalid> in_txn_limit: <invalid> serial_no: 0 } request: client: YQL_CLIENT_PGSQL stmt_id: 4920135776 schema_version: 0 targets { column_id: 12 } column_refs { ids: 12 } is_forward_scan: 1 is_aggregate: 0 limit: 1024 return_paging_state: 1 table_id: "000033e5000030008000000000004011" col_refs { column_id: 12 attno: -101 } }

edit: pasting in my additional comment immediately after the above from the internal slack thread, which I thought I had put here back then: Mtakahar 2 years ago The quickest way of fixing the wrong results is not to apply the condition on the Postgres side, but it defeats the purpose of creating an index. Proper "fix" would be to add the custom comparator support that makes a callout to the user defined comparator function or add a feature to allow users to define the binary comparable key generation function.

I replaced the existing "issue-12897.zip" with a new version: HERE. This contains a significantly simplified testcase.

Environment

I spotted the bug using YB-2.13.0.1 on my laptop (running macOS Big Sur Version 11.6.6.) I'm using a freshly-created single node cluster.

I retested in on YB-2.15.0.0. The spooled output is unchanged in all tests.

Background

Since posting the previous testcase, I have realized that using a domain that has a not null constraint risks silent wrong results. This is not considered to be a bug. See the "Notes" section in the CREATE DOMAIN section in the 11.2 PG doc. The wording is unchanged w.r.t. the "current" doc (as of this comment's timestamp)—except that “Section 5.3.1” in Version 11 becomes "Section 5.4.1" in “current”. I'm copying it here—dividing it into two sections and adding some whitespace and bolding to help the readability.

ONE

Domain constraints, particularly NOT NULL, are checked when converting a value to the domain type. It is possible for a column that is nominally of the domain type to read as NULL despite there being such a constraint. For example, this can happen in an outer-join query, if the domain column is on the nullable side of the outer join. [Another example...]

It is very difficult to avoid such problems, because of SQL's general assumption that [a column, a composite type attribute, and a PL/pgSQL formal parameter or variable, of every data type. can be NULL].

Best practice therefore is to design a domain's constraints so that NULL is allowed, and then to apply column not null constraints to columns of the domain type as needed, rather than directly to the domain type.

TWO

PostgreSQL assumes that CHECK constraints' conditions are immutable, that is, they will always give the same result for the same input value. This assumption is what justifies examining CHECK constraints only when a value is first converted to be of a domain type, and not at other times. (This is essentially the same as the treatment of table CHECK constraints, as described in Section 5.3.1.)

An example of a common way to break this assumption is to reference a user-defined function in a CHECK expression, and then to change the behavior of that function. PostgreSQL does not disallow that, but it will not notice if there are stored values of the domain type that now violate the CHECK constraint. That would cause a subsequent database dump and reload to fail. The recommended way to handle such a change is to drop the constraint (using ALTER DOMAIN), adjust the function definition, and re-add the constraint, thereby rechecking it against stored data.

Here is the best example that I could construct—based on minimum size while still demonstrating the issue:

drop domain if exists text_nn cascade;
create domain text_nn as text not null;

drop view if exists no_physical_tables cascade;
create view no_physical_tables(v, dt) as
with
  c1(v) as (
    values('dog'::text_nn)),

  c2(v) as (
    select (select v from c1 where null))

select
  v, pg_typeof(v)
from c2;

\pset null '~~~'
select v, dt from no_physical_tables;

This is the result:

  v  |   dt    
-----+---------
 ~~~ | text_nn

Self-evidently, this result shows a mutual contradiction: the column's datatype implies a not null constraint; the column is NULL. (So you must understand that point TWO in the note, about when constraints are checked, extends to cover a domain's explicit not null constraint.

When the scalar subquery "(select v from c1 where false)" is used in the CTE "c2" to assign the value to column "v", it evaluates to NULL but nevertheless inherits the data type _"textnn". This reflects a critical difference between: defining a table column using a domain that has a not null constraint; and defining it using the domain's native data type together with an explicit not null constraint. (A projected column retains the column's data type but loses attributes like constraints.)

How was the testcase changed?

All domain definitions, and their uses, were removed expect for just this:

create domain text_uc as text;

It actually does nothing except, where it's used, formally to change the data type's status from native data type to domain. It's referenced at just one spot: in set-up/3-create-strict-extract-operator.sql. Look for this:

    returns text_uc  -- As designed (provokes YB bug).
--  returns text     -- Workaround  (no YB bug)

Where appropriate, column declarations and the declarations of PL/pgSQL variables were changed to add explicit not null constraints—in line with the practice that PG doc mandates.

This makes the testcase much easier to comprehend and it removes a whole "red herring" area of possible concern.

Notice that in a real-world use case, a domain based on the native text dat type might add safe constraints that say nothing about nullness. For example, a domain isbn would apply some REGEXP magic to ensure that the value adhered properly to the formatting rules.

How did the changes affect the outcome?

As it happens, the previous version of the testcase did not depend on the prevention of NULLs. As a consequence, the test results were unchanged except in the spelling of the "Filter" and the "Index Cond" in the execution plans.

Now, these conditions are pairwise identical in PG vs YB for each of the "As designed" and "Workaround" cases, thus:

As designed: Filter: (((doc +>> 'key'::text))::text = 'dog'::text)

As designed: Index Cond: (((doc +>> 'key'::text))::text = 'dog'::text)

and:

Workaround: Filter: ((doc +>> 'key'::text) = 'dog'::text)

Workaround: Index Cond: ((doc +>> 'key'::text) = 'dog'::text)

The bug (no restriction happens when an index scan is used) still manifests in YB.

Finally... the "bonus.sql" testcase was changed in the same way. But it, too, still manifests the bug.

Still reproducible on 2.21.0

yugabyte / yugabyte-db