Обсуждение: hstore dump/restore bug in 9.3

Hi all

A user has reported a bug where a simple view using hstore does not dump
and restore correctly. I've reproduced the detailed test case they
supplied below.

The original report is by Stack Overflow user 'aidan', here:
http://stackoverflow.com/q/23599926/398670


The error is:

pg_restore: [archiver (db)] could not execute query: ERROR:  operator
does not exist: public.hstore = public.hstore
LINE 2:  SELECT NULLIF(hstore_test_table.column1, hstore_test_table....


produced by test case:



CREATE DATABASE hstore_test;

\c hstore_test

CREATE EXTENSION hstore WITH SCHEMA public;

CREATE SCHEMA hstore_test_schema;

CREATE TABLE hstore_test_schema.hstore_test_table(
   id int,
   column1 hstore,
   column2 hstore,
   PRIMARY KEY( id )
);

CREATE VIEW hstore_test_schema.hstore_test_view AS
SELECT NULLIF(column1, column2) AS comparison FROM
hstore_test_schema.hstore_test_table;



followed by command sequence:


$ pg_dump -U postgres -Fc hstore_test -f test.out
$ dropdb -U postgres hstore_test;
$ createdb -U postgres hstore_test;
$ pg_restore  -U postgres -d hstore_test test.out
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 172; 1259 96803 VIEW
hstore_test_view postgres
pg_restore: [archiver (db)] could not execute query: ERROR:  operator
does not exist: public.hstore = public.hstore
LINE 2:  SELECT NULLIF(hstore_test_table.column1, hstore_test_table....
                ^
HINT:  No operator matches the given name and argument type(s). You
might need to add explicit type casts.
    Command was: CREATE VIEW hstore_test_view AS
 SELECT NULLIF(hstore_test_table.column1, hstore_test_table.column2) AS
comparison
   FROM h...






--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 05/12/2014 10:08 AM, Craig Ringer wrote:
> Hi all
>
> A user has reported a bug where a simple view using hstore does not dump
> and restore correctly. I've reproduced the detailed test case they
> supplied below.
>
> The original report is by Stack Overflow user 'aidan', here:
> http://stackoverflow.com/q/23599926/398670
>
>
> The error is:
>
> pg_restore: [archiver (db)] could not execute query: ERROR:  operator
> does not exist: public.hstore = public.hstore
> LINE 2:  SELECT NULLIF(hstore_test_table.column1, hstore_test_table....

When running pg_restore without a DB to get an SQL dump, it's clear why
this happens - the dump sets the search_path to exclude the public
schema, which contains the hstore operators required.



CREATE EXTENSION IF NOT EXISTS hstore WITH SCHEMA public;

...

SET search_path = hstore_test_schema, pg_catalog;

...


CREATE VIEW hstore_test_view AS
 SELECT NULLIF(hstore_test_table.column1, hstore_test_table.column2) AS
comparison
   FROM hstore_test_table;



Using a different view definition makes this go away, as the original
reporter noted:

CREATE VIEW hstore_test_schema.hstore_test_view AS
SELECT column1 =  column2 AS comparison
FROM hstore_test_schema.hstore_test_table;

because the view is dumped with an explicit operator schema:

CREATE VIEW hstore_test_view AS
 SELECT (hstore_test_table.column1 OPERATOR(public.=)
hstore_test_table.column2) AS comparison
   FROM hstore_test_table;



It looks like pg_dump expects to be able to explicitly qualify operators
so it doesn't worry about setting the search_path to include them, but
it doesn't cope with operators that're used indirectly by the nullif
pseudofunction.

Do we need a way to schema-qualify the operator used in NULLIF, or to
provide an operator alias that it gets dumped as?

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 05/12/2014 10:18 AM, Craig Ringer wrote:
> Do we need a way to schema-qualify the operator used in NULLIF, or to
> provide an operator alias that it gets dumped as?

A bit more digging shows that this doesn't affect LEAST or GREATEST,
which I'd expect it to as well. Nor DISTINCT (otherwise someone would've
yelled long ago).

LEAST and GREATEST look up the operators from the b-tree opclass for the
type(s) they're operating on using the type cache. So they don't care
about search_path, they respect the schema of the base type they're
operating on, per the case for T_MinMaxExpr in executor/execQual.c .

NULLIF does not do so, per the case in T_NullIfExpr. But we don't
actually get that far.

The immediate failure is actually:

hstore_test=# \set VERBOSITY verbose
hstore_test=# CREATE VIEW hstore_test_schema.hstore_test_view AS
SELECT NULLIF(column1, column2) AS comparison FROM
hstore_test_schema.hstore_test_table;
ERROR:  42883: operator does not exist: public.hstore = public.hstore
LINE 2: SELECT NULLIF(column1, column2) AS comparison FROM hstore_te...
               ^
HINT:  No operator matches the given name and argument type(s). You
might need to add explicit type casts.
LOCATION:  op_error, parse_oper.c:722


which dies in:

#2  0x000000000051ad34 in make_op (pstate=pstate@entry=0x1189f38,
opname=0x1189c10, ltree=ltree@entry=0x118a528, rtree=0x118a590,
location=58) at parse_oper.c:770
#3  0x00000000005155e1 in transformAExprNullIf (a=0x1189bc0,
pstate=0x1189f38) at parse_expr.c:1021

instead of using the b-tree opclass of the base type to find the operator.

This looks like a bug that'll want backpatching and it's not a data loss
risk, so I don't think it's urgent to shove this in before 9.4 closes.
I'll look for time to try out a possible fix soon, after reading all the
code that shows how DISTINCT and GREATEST/LEAST handle this in detail.
I'm hard up against a deadline at the moment so it'll be a little while.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig@2ndquadrant.com> writes:
> On 05/12/2014 10:18 AM, Craig Ringer wrote:
>> Do we need a way to schema-qualify the operator used in NULLIF, or to
>> provide an operator alias that it gets dumped as?

> This looks like a bug that'll want backpatching and it's not a data loss
> risk, so I don't think it's urgent to shove this in before 9.4 closes.

FWIW, I don't think this is something we ought to back-patch.  NULLIF
is effectively defined in terms of "a = b", where the = operator is
whatever would be found by operator lookup.  It's probably better to
look for a default btree opclass, but that's a different behavior;
and one that will have its own failure modes, particularly if a and b
aren't of the same type.

It is worth noting that the SQL standard says in so many words:

    NULLIF (V1, V2) is equivalent to the following <case specification>:
    CASE WHEN V1=V2 THEN NULL ELSE V1 END

Now, you can argue about how literally they mean that; for one thing
I doubt they want you to evaluate V1 twice.  But nonetheless the
construct is defined in terms of an operator named "=".

Also, I think we still have some other dependencies on assumed
operator names in eg. CASE.  Cleaning up only NULLIF may not be a
full solution.

There have been past discussions of whether we ought to follow the
letter or the spirit of the standard's references to "=" and other
operators.  Don't recall if we came to any definitive conclusions,
but right now we're clearly not totally consistent on this point.

            regards, tom lane

On 05/12/2014 11:48 AM, Tom Lane wrote:
> Also, I think we still have some other dependencies on assumed
> operator names in eg. CASE.  Cleaning up only NULLIF may not be a
> full solution.

GREATEST(..) and LEAST(..) already handle this, as does DISTINCT(...).

Is NULLIF different? Or are they doing something suspect, just
differently suspect?

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig@2ndquadrant.com> writes:
> On 05/12/2014 11:48 AM, Tom Lane wrote:
>> Also, I think we still have some other dependencies on assumed
>> operator names in eg. CASE.  Cleaning up only NULLIF may not be a
>> full solution.

> GREATEST(..) and LEAST(..) already handle this, as does DISTINCT(...).

Those are different in that they're comparing multiple values that are
always of the same datatype, so that "look for the default btree opclass
for that type" is a well-defined rule.  NULLIF and CASE will certainly
require additional thought.  I think IN has the issue as well.

            regards, tom lane