Skip to content

Commit

Permalink
Fix SP-GiST scan initialization logic for binary-compatible cases.
Browse files Browse the repository at this point in the history
Commit ac9099f rearranged the logic in spgGetCache() that determines
the index's attType (nominal input data type) and leafType (actual
type stored in leaf index tuples).  Turns out this broke things for
the case where (a) the actual input data type is different from the
nominal type, (b) the opclass's config function leaves leafType
defaulted, and (c) the opclass has no "compress" function.  (b) caused
us to assign the actual input data type as leafType, and then since
that's not attType, we complained that a "compress" function is
required.  For non-polymorphic opclasses, condition (a) arises in
binary-compatible cases, such as using SP-GiST text_ops for a varchar
column, or using any opclass on a domain over its nominal input type.

To fix, use attType for leafType when the index's declared column type
is different from but binary-compatible with attType.  Do this only in
the defaulted-leafType case, to avoid overriding any explicit
selection made by the opclass.

Per bug #17294 from Ilya Anfimov.  Back-patch to v14.

Discussion: https://postgr.es/m/17294-8f6c7962ce877edc@postgresql.org
  • Loading branch information
tglsfdc committed Nov 20, 2021
1 parent ead49eb commit 6d07cbc
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 0 deletions.
12 changes: 12 additions & 0 deletions src/backend/access/spgist/spgutils.c
Expand Up @@ -25,6 +25,7 @@
#include "catalog/pg_amop.h"
#include "commands/vacuum.h"
#include "nodes/nodeFuncs.h"
#include "parser/parse_coerce.h"
#include "storage/bufmgr.h"
#include "storage/indexfsm.h"
#include "storage/lmgr.h"
Expand Down Expand Up @@ -218,9 +219,20 @@ spgGetCache(Relation index)
* correctly, so believe leafType if it's given.)
*/
if (!OidIsValid(cache->config.leafType))
{
cache->config.leafType =
TupleDescAttr(RelationGetDescr(index), spgKeyColumn)->atttypid;

/*
* If index column type is binary-coercible to atttype (for
* example, it's a domain over atttype), treat it as plain atttype
* to avoid thinking we need to compress.
*/
if (cache->config.leafType != atttype &&
IsBinaryCoercible(cache->config.leafType, atttype))
cache->config.leafType = atttype;
}

/* Get the information we need about each relevant datatype */
fillTypeDesc(&cache->attType, atttype);

Expand Down
21 changes: 21 additions & 0 deletions src/test/regress/expected/spgist.out
Expand Up @@ -65,3 +65,24 @@ DETAIL: Valid values are between "10" and "100".
-- Modify fillfactor in existing index
alter index spgist_point_idx set (fillfactor = 90);
reindex index spgist_point_idx;
-- Test index over a domain
create domain spgist_text as varchar;
create table spgist_domain_tbl (f1 spgist_text);
create index spgist_domain_idx on spgist_domain_tbl using spgist(f1);
insert into spgist_domain_tbl values('fee'), ('fi'), ('fo'), ('fum');
explain (costs off)
select * from spgist_domain_tbl where f1 = 'fo';
QUERY PLAN
-----------------------------------------------
Bitmap Heap Scan on spgist_domain_tbl
Recheck Cond: ((f1)::text = 'fo'::text)
-> Bitmap Index Scan on spgist_domain_idx
Index Cond: ((f1)::text = 'fo'::text)
(4 rows)

select * from spgist_domain_tbl where f1 = 'fo';
f1
----
fo
(1 row)

9 changes: 9 additions & 0 deletions src/test/regress/sql/spgist.sql
Expand Up @@ -71,3 +71,12 @@ create index spgist_point_idx2 on spgist_point_tbl using spgist(p) with (fillfac
-- Modify fillfactor in existing index
alter index spgist_point_idx set (fillfactor = 90);
reindex index spgist_point_idx;

-- Test index over a domain
create domain spgist_text as varchar;
create table spgist_domain_tbl (f1 spgist_text);
create index spgist_domain_idx on spgist_domain_tbl using spgist(f1);
insert into spgist_domain_tbl values('fee'), ('fi'), ('fo'), ('fum');
explain (costs off)
select * from spgist_domain_tbl where f1 = 'fo';
select * from spgist_domain_tbl where f1 = 'fo';

0 comments on commit 6d07cbc

Please sign in to comment.