Thread: Assertion failure in get_attstatsslot()

Consider the following small testcase:

BEGIN;

CREATE OR REPLACE FUNCTION upper(IN varchar, OUT varchar)
LANGUAGE SQL STRICT IMMUTABLE
AS
$$       SELECT pg_catalog.upper($1)::varchar;
$$;

CREATE TABLE foo(value varchar);

INSERT INTO foo SELECT 'helmle' FROM generate_series(1, 1000);

CREATE INDEX foo_upper_index ON foo(public.UPPER(value));

ANALYZE foo;

-- assertion failure
EXPLAIN SELECT 1 FROM foo WHERE UPPER(value) = 'xyz';

COMMIT;

While the sense of the UPPER() function is surely debatable (but it is used 
in this way in the database where the assertion failure was triggered), the 
EXPLAIN in the script crashes with

TRAP: FailedAssertion("!(((array)->elemtype) == elmtype)", File: 
"arrayfuncs.c", Line: 2970)
LOG:  server process (PID 15451) was terminated by signal 6: Abort trap

It took me a while to strip that down to this script, because it only 
happens when the statistics slots of the index are examined for the 
constant value in the where clause. The stavalues array holds type oid 
1043, whereas type oid 25 is expected.

I tried it back from current -HEAD to 8.3.11 and managed to reproduce it 
everywhere. Non-cassert builds are working correctly, so i'm not sure 
wether this is an over-aggressive assert() or masks a problem from 
somewhere else.

-- 
Thanks
Bernd

Bernd Helmle <mailings@oopsware.de> writes:
> -- assertion failure
> EXPLAIN SELECT 1 FROM foo WHERE UPPER(value) = 'xyz';

> I tried it back from current -HEAD to 8.3.11 and managed to reproduce it 
> everywhere. Non-cassert builds are working correctly, so i'm not sure 
> wether this is an over-aggressive assert() or masks a problem from 
> somewhere else.

I can reproduce it back to 8.0, and I'm not sure it couldn't be done in
older branches with a slightly different test case :-(.

I think that at bottom the issue is that the planner plays fast and
loose with binary-compatible data types by being willing to strip off
RelabelType nodes whenever it's trying to arrive at selectivity
estimates.  This isn't something we can easily change, though.

I'm quite hesitant to remove that Assert from deconstruct_array(), but
it strikes me that an easy workaround is to make get_attstatsslot()
use ARR_ELEMTYPE(statarray) rather than the passed-in atttype.  This
at least moves the fast-and-looseness into a function that's basically
a creature of the planner, rather than fooling with a fundamental
utility like deconstruct_array().

If anybody feels really uncomfortable with that, we could add a
compensating "Assert(IsBinaryCoercible(ARR_ELEMTYPE(statarray),
atttype))" into get_attstatsslot().  Not sure if it's worth the cycles.
        regards, tom lane

Excerpts from Tom Lane's message of vie jul 09 12:16:42 -0400 2010:

> If anybody feels really uncomfortable with that, we could add a
> compensating "Assert(IsBinaryCoercible(ARR_ELEMTYPE(statarray),
> atttype))" into get_attstatsslot().  Not sure if it's worth the cycles.

Cycles spent only in assert-enabled builds?  Why not?

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Tom Lane's message of vie jul 09 12:16:42 -0400 2010:
>> If anybody feels really uncomfortable with that, we could add a
>> compensating "Assert(IsBinaryCoercible(ARR_ELEMTYPE(statarray),
>> atttype))" into get_attstatsslot().  Not sure if it's worth the cycles.

> Cycles spent only in assert-enabled builds?  Why not?

I decided not to do it, not so much because of any performance worries
as that I didn't want to make lsyscache.c depend on the parser.  Maybe
sometime we should rethink where the parse_coerce.c functionality lives.
        regards, tom lane

1;2401;0cOn Fri, Jul 09, 2010 at 06:49:27PM -0400, Alvaro Herrera wrote:
> Excerpts from Tom Lane's message of vie jul 09 12:16:42 -0400 2010:
>
> > If anybody feels really uncomfortable with that, we could add a
> > compensating "Assert(IsBinaryCoercible(ARR_ELEMTYPE(statarray),
> > atttype))" into get_attstatsslot().  Not sure if it's worth the cycles.
> Cycles spent only in assert-enabled builds?  Why not?
The slower assert-enabled is, the less likely it is that somebody can
run serious testing on it - potentially catching bugs way much easier.
Contrarily to your statement I would actually like to remove some
older asserts.
For example AtEOXact_Buffers makes it significantly expensive to do
assert tests on larger shbuf setups.

Andres