Re: BUG #5705: btree_gist: Index on inet changes query result

The following bug has been logged online:

Bug reference: 5705
Logged by: Andreas Karlsson
Email address: andreas(at)proxel(dot)se
PostgreSQL version: 9.1
Operating system: Linux
Description: btree_gist: Index on inet changes query result
Details:

Hi,

I was looking at the code to see how one would improve indexing of the inet
types and saw an inconsistency between the compressed format
(gbt_inet_compress) and how network_cmp_internal works. The btree_gist
module ignores the netmask.

This means that while the operator thinks 1.255.255.200/8 is smaller than
1.0.0.0 the GiST index thinks the opposite.

An example for how to reproduce the bug:

-- Demostrate that I did not get the operator wrong. :)
SELECT '1.255.255.200/8'::inet < '1.0.0.0'::inet;
?column?
----------
t
(1 row)

-- Create and populate table
CREATE TABLE inet_test (a inet);
INSERT INTO inet_test VALUES ('1.255.255.200/8');

-- Without index
SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
a
-----------------
1.255.255.200/8
(1 row)

EXPLAIN SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
QUERY PLAN
-------------------------------------------------------------
Seq Scan on inet_test (cost=0.00..26.38 rows=437 width=32)
Filter: (a < '1.0.0.0'::inet)
(2 rows)

-- With index
CREATE INDEX inet_test_idx ON inet_test USING gist (a);
SET enable_seqscan = false;

SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
a
---
(0 rows)

EXPLAIN SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
QUERY PLAN
----------------------------------------------------------------------------
----
Index Scan using inet_test_idx on inet_test (cost=0.00..8.27 rows=1
width=32)
Index Cond: (a < '1.0.0.0'::inet)
(2 rows)

-- With btree index
DROP INDEX inet_test_idx;
CREATE INDEX inet_test_btree_idx ON inet_test USING btree (a);
SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
a
-----------------
1.255.255.200/8
(1 row)

EXPLAIN SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
QUERY PLAN

----------------------------------------------------------------------------
----
Index Scan using inet_test_btree_idx on inet_test (cost=0.00..8.27 rows=1
width=32)
Index Cond: (a < '1.0.0.0'::inet)
(2 rows)

"Andreas Karlsson" <andreas(at)proxel(dot)se> writes:
> I was looking at the code to see how one would improve indexing of the inet
> types and saw an inconsistency between the compressed format
> (gbt_inet_compress) and how network_cmp_internal works. The btree_gist
> module ignores the netmask.

Well, actually the btree_gist implementation for inet is a completely
broken piece of junk: it thinks that convert_network_to_scalar is 100%
trustworthy and can be used as a substitute for the real comparison
functions, which isn't even approximately true. I'm not sure why
Teodor implemented it like that instead of using the type's real
comparison functions, but it's pretty much useless if you want the
same sort order that the type itself defines.

regards, tom lane

On Mon, Oct 11, 2010 at 7:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Andreas Karlsson" <andreas(at)proxel(dot)se> writes:
>> I was looking at the code to see how one would improve indexing of the inet
>> types and saw an inconsistency between the compressed format
>> (gbt_inet_compress) and how network_cmp_internal works. The btree_gist
>> module ignores the netmask.
>
> Well, actually the btree_gist implementation for inet is a completely
> broken piece of junk: it thinks that convert_network_to_scalar is 100%
> trustworthy and can be used as a substitute for the real comparison
> functions, which isn't even approximately true.  I'm not sure why
> Teodor implemented it like that instead of using the type's real
> comparison functions, but it's pretty much useless if you want the
> same sort order that the type itself defines.

Are you planning to fix this?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Oct 11, 2010 at 7:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Well, actually the btree_gist implementation for inet is a completely
>> broken piece of junk: it thinks that convert_network_to_scalar is 100%
>> trustworthy and can be used as a substitute for the real comparison
>> functions, which isn't even approximately true.

> Are you planning to fix this?

No. I don't understand why Teodor did it like that, so I'm not going
to try to change it. I'd be willing to take responsibility for ripping
out btree_gist's inet support altogether ...

regards, tom lane

On Tue, 2010-10-19 at 18:22 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Oct 11, 2010 at 7:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Well, actually the btree_gist implementation for inet is a completely
> >> broken piece of junk: it thinks that convert_network_to_scalar is 100%
> >> trustworthy and can be used as a substitute for the real comparison
> >> functions, which isn't even approximately true.
>
> > Are you planning to fix this?
>
> No. I don't understand why Teodor did it like that, so I'm not going
> to try to change it. I'd be willing to take responsibility for ripping
> out btree_gist's inet support altogether ...
>
> regards, tom lane

That is the reason why I just reported it instead of trying to fix it
myself first. Since I could not understand why it was done like that, I
did not feel like fixing it.

Best regards,
Andreas Karlsson

Teodor, would you please comment on this bug after reading the entire
thread which includes comments from other developers?

http://archives.postgresql.org/pgsql-bugs/2010-10/msg00099.php

Thanks.

---------------------------------------------------------------------------

Andreas Karlsson wrote:
>
> The following bug has been logged online:
>
> Bug reference: 5705
> Logged by: Andreas Karlsson
> Email address: andreas(at)proxel(dot)se
> PostgreSQL version: 9.1
> Operating system: Linux
> Description: btree_gist: Index on inet changes query result
> Details:
>
> Hi,
>
> I was looking at the code to see how one would improve indexing of the inet
> types and saw an inconsistency between the compressed format
> (gbt_inet_compress) and how network_cmp_internal works. The btree_gist
> module ignores the netmask.
>
> This means that while the operator thinks 1.255.255.200/8 is smaller than
> 1.0.0.0 the GiST index thinks the opposite.
>
> An example for how to reproduce the bug:
>
> -- Demostrate that I did not get the operator wrong. :)
> SELECT '1.255.255.200/8'::inet < '1.0.0.0'::inet;
> ?column?
> ----------
> t
> (1 row)
>
> -- Create and populate table
> CREATE TABLE inet_test (a inet);
> INSERT INTO inet_test VALUES ('1.255.255.200/8');
>
>
> -- Without index
> SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
> a
> -----------------
> 1.255.255.200/8
> (1 row)
>
> EXPLAIN SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
> QUERY PLAN
> -------------------------------------------------------------
> Seq Scan on inet_test (cost=0.00..26.38 rows=437 width=32)
> Filter: (a < '1.0.0.0'::inet)
> (2 rows)
>
> -- With index
> CREATE INDEX inet_test_idx ON inet_test USING gist (a);
> SET enable_seqscan = false;
>
> SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
> a
> ---
> (0 rows)
>
> EXPLAIN SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
> QUERY PLAN
> ----------------------------------------------------------------------------
> ----
> Index Scan using inet_test_idx on inet_test (cost=0.00..8.27 rows=1
> width=32)
> Index Cond: (a < '1.0.0.0'::inet)
> (2 rows)
>
> -- With btree index
> DROP INDEX inet_test_idx;
> CREATE INDEX inet_test_btree_idx ON inet_test USING btree (a);
> SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
> a
> -----------------
> 1.255.255.200/8
> (1 row)
>
> EXPLAIN SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
> QUERY PLAN
>
> ----------------------------------------------------------------------------
> ----
> Index Scan using inet_test_btree_idx on inet_test (cost=0.00..8.27 rows=1
> width=32)
> Index Cond: (a < '1.0.0.0'::inet)
> (2 rows)
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

This is currently a TODO so at least we are tracking it.

---------------------------------------------------------------------------

andreas wrote:
> On Tue, 2010-10-19 at 18:22 -0400, Tom Lane wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > On Mon, Oct 11, 2010 at 7:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >> Well, actually the btree_gist implementation for inet is a completely
> > >> broken piece of junk: it thinks that convert_network_to_scalar is 100%
> > >> trustworthy and can be used as a substitute for the real comparison
> > >> functions, which isn't even approximately true.
> >
> > > Are you planning to fix this?
> >
> > No. I don't understand why Teodor did it like that, so I'm not going
> > to try to change it. I'd be willing to take responsibility for ripping
> > out btree_gist's inet support altogether ...
> >
> > regards, tom lane
>
> That is the reason why I just reported it instead of trying to fix it
> myself first. Since I could not understand why it was done like that, I
> did not feel like fixing it.
>
> Best regards,
> Andreas Karlsson
>
>
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +