Re: (possible) bug with constraint exclusion

Hi ,

looks like constraint exclusion is being too aggressive in excluding null values
although its well known that check constraints apply on not null values only.
Hope the minimal test session below explains the problem we facing.
BTW: we are very impressed with the performance gains we achieved by
partitioning a table recently.

tradein_clients=> SELECT version();
version
---------------------------------------------------------------------------------------------------
PostgreSQL 8.2.6 on i686-pc-linux-gnu, compiled by GCC gcc (GCC)
3.4.6 20060404 (Red Hat 3.4.6-9)
(1 row)

tradein_clients=> \pset null NULL

tradein_clients=> \d x
Table "temp.x"
Column | Type | Modifiers
--------+---------+-----------
id | integer |
Check constraints:
"x_id_check" CHECK (id > 0)

tradein_clients=> SELECT * from x;
id
------
1
2
NULL
(3 rows)

tradein_clients=> explain SELECT * from x where id is null;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: false
(2 rows)

tradein_clients=> SELECT * from x where id is null;
id
----
(0 rows)
tradein_clients=> SET constraint_exclusion to off;
SET
tradein_clients=> SELECT * from x where id is null;
id
------
NULL
(1 row)

tradein_clients=>

Regds
mallah.

Update the phenomenon does not exists in 8.2.0 but exists in 8.2.5.

On Jan 11, 2008 12:28 PM, Rajesh Kumar Mallah <mallah(dot)rajesh(at)gmail(dot)com> wrote:
> Hi ,
>
> looks like constraint exclusion is being too aggressive in excluding null values
> although its well known that check constraints apply on not null values only.
> Hope the minimal test session below explains the problem we facing.
> BTW: we are very impressed with the performance gains we achieved by
> partitioning a table recently.
>
>
>
> tradein_clients=> SELECT version();
> version
> ---------------------------------------------------------------------------------------------------
> PostgreSQL 8.2.6 on i686-pc-linux-gnu, compiled by GCC gcc (GCC)
> 3.4.6 20060404 (Red Hat 3.4.6-9)
> (1 row)
>
> tradein_clients=> \pset null NULL
>
>
> tradein_clients=> \d x
> Table "temp.x"
> Column | Type | Modifiers
> --------+---------+-----------
> id | integer |
> Check constraints:
> "x_id_check" CHECK (id > 0)
>
> tradein_clients=> SELECT * from x;
> id
> ------
> 1
> 2
> NULL
> (3 rows)
>
> tradein_clients=> explain SELECT * from x where id is null;
> QUERY PLAN
> ------------------------------------------
> Result (cost=0.00..0.01 rows=1 width=0)
> One-Time Filter: false
> (2 rows)
>
> tradein_clients=> SELECT * from x where id is null;
> id
> ----
> (0 rows)
> tradein_clients=> SET constraint_exclusion to off;
> SET
> tradein_clients=> SELECT * from x where id is null;
> id
> ------
> NULL
> (1 row)
>
> tradein_clients=>
>
> Regds
> mallah.
>

"Rajesh Kumar Mallah" <mallah(dot)rajesh(at)gmail(dot)com> writes:
> looks like constraint exclusion is being too aggressive in excluding null values

Hmm, you're right. Looks like I broke it here:
http://archives.postgresql.org/pgsql-committers/2007-05/msg00187.php

> although its well known that check constraints apply on not null values only.

No, that is not a correct statement either --- it's exactly that type of
sloppy thinking that got me into trouble with this patch :-(

The problem is that predicate_refuted_by_simple_clause() is failing to
distinguish whether "refutes" means "proves false" or "proves not true".
For constraint exclusion we have to use the stricter "proves false"
interpretation, and in that scenario a clause "foo IS NULL" fails to
refute a check constraint "foo > 0", because the latter will produce
NULL which isn't false and therefore doesn't cause the check constraint
to fail.

The motivation for that patch was to support IS NULL as one partition
of a partitioned table. Thinking about it I see that if the other
partitions have check constraints like "foo > 0" then the partitioning
is actually incorrect, because the other check constraints are failing
to exclude NULLs. The right way to set up such a partitioned table is
to include "foo IS NOT NULL" as part of the check constraint, or as
a special-purpose NOT NULL flag, except in the IS NULL partition.
The current constraint exclusion logic fails to notice attnotnull,
though. So the correct fix seems to be:

* Fix predicate_refuted_by_simple_clause to not suppose that a strict
operator is proved FALSE by an IS NULL clause.

* Fix relation_excluded_by_constraints to add "foo IS NOT NULL" clauses
to the constraint list for attnotnull columns (perhaps this should be
pushed into get_relation_constraints?). This buys back the loss of
exclusion from the other change, so long as the partitioning is done
correctly.

regards, tom lane

On Jan 12, 2008 1:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Rajesh Kumar Mallah" <mallah(dot)rajesh(at)gmail(dot)com> writes:
> > looks like constraint exclusion is being too aggressive in excluding null values
>
> Hmm, you're right. Looks like I broke it here:
> http://archives.postgresql.org/pgsql-committers/2007-05/msg00187.php
>
> > although its well known that check constraints apply on not null values only.
>
> No, that is not a correct statement either --- it's exactly that type of
> sloppy thinking that got me into trouble with this patch :-(
>
> The problem is that predicate_refuted_by_simple_clause() is failing to
> distinguish whether "refutes" means "proves false" or "proves not true".
> For constraint exclusion we have to use the stricter "proves false"
> interpretation, and in that scenario a clause "foo IS NULL" fails to
> refute a check constraint "foo > 0", because the latter will produce
> NULL which isn't false and therefore doesn't cause the check constraint
> to fail.
>
> The motivation for that patch was to support IS NULL as one partition
> of a partitioned table. Thinking about it I see that if the other
> partitions have check constraints like "foo > 0" then the partitioning
> is actually incorrect, because the other check constraints are failing
> to exclude NULLs. The right way to set up such a partitioned table is
> to include "foo IS NOT NULL" as part of the check constraint, or as
> a special-purpose NOT NULL flag, except in the IS NULL partition.
> The current constraint exclusion logic fails to notice attnotnull,
> though. So the correct fix seems to be:

Dear Tom,
Thanks for the elaborate explanation on your part,
owing to my limitations I could not understand all the parts of it.
Am I correct in understanding that the current behavior is inappropriate
and shall be corrected at some point of time in future versions ?
thanks once again to all the developers for making PostgreSQL.

regds
mallah.

>
> * Fix predicate_refuted_by_simple_clause to not suppose that a strict
> operator is proved FALSE by an IS NULL clause.
>
> * Fix relation_excluded_by_constraints to add "foo IS NOT NULL" clauses
> to the constraint list for attnotnull columns (perhaps this should be
> pushed into get_relation_constraints?). This buys back the loss of
> exclusion from the other change, so long as the partitioning is done
> correctly.
>
> regards, tom lane
>

"Rajesh Kumar Mallah" <mallah(dot)rajesh(at)gmail(dot)com> writes:
> Am I correct in understanding that the current behavior is inappropriate
> and shall be corrected at some point of time in future versions ?

It's a bug, it's patched:
http://archives.postgresql.org/pgsql-committers/2008-01/msg00184.php

regards, tom lane

On Jan 12, 2008 10:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Rajesh Kumar Mallah" <mallah(dot)rajesh(at)gmail(dot)com> writes:
> > Am I correct in understanding that the current behavior is inappropriate
> > and shall be corrected at some point of time in future versions ?
>
> It's a bug, it's patched:
> http://archives.postgresql.org/pgsql-committers/2008-01/msg00184.php

Thanks for the (unbelievable) quick response,

I applied the patch on my production server and the problem is gone.
tradein_clients=> \pset null NULL
Null display is "NULL".
tradein_clients=> SELECT * from temp.x where id is NULL;
id
------
NULL
(1 row)

tradein_clients=> SELECT * from temp.x ;
id
------
1
2
NULL
(3 rows)

tradein_clients=>

Regds
mallah.

>
> regards, tom lane
>

Hi list,
> It's a bug, it's patched:
> http://archives.postgresql.org/pgsql-committers/2008-01/msg00184.php
>
I have just stumbled on the same bug today and was very happy to find a
patch; however, I have a 8.2.6 server running which of course cannot be
patched. (According to the CVS tags the revisions of plancat.c and
predtest.c in the 8.2.6 release are 1.127 and 1.10.2.2, respectively,
whereas the patch is against the head revisions.)
Would it be possible to provide a patch against the 8.2.6 stable release
(the one that can be downloaded from the download section of the
homepage)? It seems as if partitioning is unusable without this patch
because when constraint exclusion is enabled, records cannot be found by
select or update even in tables where partitioning is not realized at
all. On the other hand, without constraint exclusion, partitioning at
all doesn't make sense (as far as I understand).
Thanks a lot for your help!

Regards,
Christian

--
Deriva GmbH Tel.: +49 551 489500-42
Financial IT and Consulting Fax: +49 551 489500-91
Hans-Böckler-Straße 2 http://www.deriva.de
D-37079 Göttingen

Deriva CA Certificate: http://www.deriva.de/deriva-ca.cer

=?ISO-8859-1?Q?Christian_Schr=F6der?= <cs(at)deriva(dot)de> writes:
>> It's a bug, it's patched:
>> http://archives.postgresql.org/pgsql-committers/2008-01/msg00184.php
>>
> I have just stumbled on the same bug today and was very happy to find a
> patch; however, I have a 8.2.6 server running which of course cannot be
> patched. (According to the CVS tags the revisions of plancat.c and
> predtest.c in the 8.2.6 release are 1.127 and 1.10.2.2, respectively,
> whereas the patch is against the head revisions.)

Better look again.

regards, tom lane

Tom Lane wrote:
> Better look again.
>
Sounds like a sensible advice ... I somehow managed to find
http://archives.postgresql.org/pgsql-committers/2008-01/msg00183.php
instead of
http://archives.postgresql.org/pgsql-committers/2008-01/msg00184.php ...

Sorry for that!

Regards,
Christian

--
Deriva GmbH Tel.: +49 551 489500-42
Financial IT and Consulting Fax: +49 551 489500-91
Hans-Böckler-Straße 2 http://www.deriva.de
D-37079 Göttingen

Deriva CA Certificate: http://www.deriva.de/deriva-ca.cer