Re: GiST operator class for bool
On 12/6/21 22:35, Pavel Luzanov wrote: Hello, I don't see any changes in the documentation.[1] Should bool appear in the looong list of supported operator classes? You're right, I forgot to update the list of data types in the docs. Fixed, thanks for the report. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: GiST operator class for bool
Hello, I don't see any changes in the documentation.[1] Should bool appear in the looong list of supported operator classes? [1] https://www.postgresql.org/docs/devel/btree-gist.html -- Pavel Luzanov Postgres Professional: https://postgrespro.com The Russian Postgres Company
Re: GiST operator class for bool
On 11/7/21 20:53, Tomas Vondra wrote: Hi, On 11/7/21 17:44, Tom Lane wrote: Tomas Vondra writes: Pushed, after adding some simple EXPLAIN to the regression test. skink is reporting that this has some valgrind issues [1]. I suspect sloppy conversion between bool and Datum, but didn't go looking. It's actually a bit worse than that :-( The opclass is somewhat confused about the type it should use for storage. The gbtree_ninfo struct says it's using gbtreekey4, the SQL script claims the params are gbtreekey8, and it should actually use gbtreekey2. Sorry for not noticing that. The attached patch fixes the valgrind error for me. I've pushed the fix, hopefully that'll make skink happy. What surprised me a bit is that the opclass used gbtreekey4 storage, the equality support proc was defined as using gbtreekey8 FUNCTION7gbt_bool_same (gbtreekey8, gbtreekey8, internal), yet the gistvalidate() did not report this. Turns out this is because ok = check_amproc_signature(procform->amproc, INTERNALOID, false, 3, 3, opckeytype, opckeytype, INTERNALOID); i.e. with exact=false, so these type differences are ignored. Changing it to true reports the issue (and no other issues in check-world). But maybe there are reasons to keep using false? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Companydiff --git a/src/backend/access/gist/gistvalidate.c b/src/backend/access/gist/gistvalidate.c index b885fa2b25..31cc4f0a21 100644 --- a/src/backend/access/gist/gistvalidate.c +++ b/src/backend/access/gist/gistvalidate.c @@ -131,7 +131,7 @@ gistvalidate(Oid opclassoid) 2, 2, INTERNALOID, INTERNALOID); break; case GIST_EQUAL_PROC: -ok = check_amproc_signature(procform->amproc, INTERNALOID, false, +ok = check_amproc_signature(procform->amproc, INTERNALOID, true, 3, 3, opckeytype, opckeytype, INTERNALOID); break;
Re: GiST operator class for bool
Hi,
On 11/7/21 17:44, Tom Lane wrote:
Tomas Vondra writes:
Pushed, after adding some simple EXPLAIN to the regression test.
skink is reporting that this has some valgrind issues [1].
I suspect sloppy conversion between bool and Datum, but
didn't go looking.
It's actually a bit worse than that :-( The opclass is somewhat confused
about the type it should use for storage. The gbtree_ninfo struct says
it's using gbtreekey4, the SQL script claims the params are gbtreekey8,
and it should actually use gbtreekey2. Sorry for not noticing that.
The attached patch fixes the valgrind error for me.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/contrib/btree_gist/btree_bool.c b/contrib/btree_gist/btree_bool.c
index 25ce1e2b77..1be246ea5e 100644
--- a/contrib/btree_gist/btree_bool.c
+++ b/contrib/btree_gist/btree_bool.c
@@ -72,7 +72,7 @@ static const gbtree_ninfo tinfo =
{
gbt_t_bool,
sizeof(bool),
- 4, /* sizeof(gbtreekey4) */
+ 2, /* sizeof(gbtreekey2) */
gbt_boolgt,
gbt_boolge,
gbt_booleq,
diff --git a/contrib/btree_gist/btree_gist--1.6--1.7.sql b/contrib/btree_gist/btree_gist--1.6--1.7.sql
index 0e68356982..1085216501 100644
--- a/contrib/btree_gist/btree_gist--1.6--1.7.sql
+++ b/contrib/btree_gist/btree_gist--1.6--1.7.sql
@@ -4,6 +4,21 @@
\echo Use "ALTER EXTENSION btree_gist UPDATE TO '1.7'" to load this file. \quit
-- This upgrade scripts add support for bool.
+CREATE FUNCTION gbtreekey2_in(cstring)
+RETURNS gbtreekey2
+AS 'MODULE_PATHNAME', 'gbtreekey_in'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE FUNCTION gbtreekey2_out(gbtreekey2)
+RETURNS cstring
+AS 'MODULE_PATHNAME', 'gbtreekey_out'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE TYPE gbtreekey2 (
+ INTERNALLENGTH = 2,
+ INPUT = gbtreekey2_in,
+ OUTPUT = gbtreekey2_out
+);
-- Define the GiST support methods
CREATE FUNCTION gbt_bool_consistent(internal,bool,int2,oid,internal)
@@ -32,11 +47,11 @@ AS 'MODULE_PATHNAME'
LANGUAGE C IMMUTABLE STRICT;
CREATE FUNCTION gbt_bool_union(internal, internal)
-RETURNS gbtreekey8
+RETURNS gbtreekey2
AS 'MODULE_PATHNAME'
LANGUAGE C IMMUTABLE STRICT;
-CREATE FUNCTION gbt_bool_same(gbtreekey8, gbtreekey8, internal)
+CREATE FUNCTION gbt_bool_same(gbtreekey2, gbtreekey2, internal)
RETURNS internal
AS 'MODULE_PATHNAME'
LANGUAGE C IMMUTABLE STRICT;
@@ -57,6 +72,6 @@ AS
FUNCTION 4 gbt_decompress (internal),
FUNCTION 5 gbt_bool_penalty (internal, internal, internal),
FUNCTION 6 gbt_bool_picksplit (internal, internal),
- FUNCTION 7 gbt_bool_same (gbtreekey8, gbtreekey8, internal),
+ FUNCTION 7 gbt_bool_same (gbtreekey2, gbtreekey2, internal),
FUNCTION 9 gbt_bool_fetch (internal),
- STORAGE gbtreekey8;
+ STORAGE gbtreekey2;
Re: GiST operator class for bool
Tomas Vondra writes: > Pushed, after adding some simple EXPLAIN to the regression test. skink is reporting that this has some valgrind issues [1]. I suspect sloppy conversion between bool and Datum, but didn't go looking. ==1805451== VALGRINDERROR-BEGIN ==1805451== Uninitialised byte(s) found during client check request ==1805451==at 0x59EFEA: PageAddItemExtended (bufpage.c:346) ==1805451==by 0x2100B9: gistfillbuffer (gistutil.c:46) ==1805451==by 0x2050F9: gistplacetopage (gist.c:562) ==1805451==by 0x20546B: gistinserttuples (gist.c:1277) ==1805451==by 0x205BB5: gistinserttuple (gist.c:1230) ==1805451==by 0x206067: gistdoinsert (gist.c:885) ==1805451==by 0x2084FB: gistBuildCallback (gistbuild.c:829) ==1805451==by 0x23B572: heapam_index_build_range_scan (heapam_handler.c:1694) ==1805451==by 0x208E7D: table_index_build_scan (tableam.h:1756) ==1805451==by 0x208E7D: gistbuild (gistbuild.c:309) ==1805451==by 0x2D10C8: index_build (index.c:2983) ==1805451==by 0x2D2A7D: index_create (index.c:1232) ==1805451==by 0x383E67: DefineIndex (indexcmds.c:1128) ==1805451== Address 0x10cab1e4 is 12 bytes inside a block of size 16 client-defined ==1805451==at 0x712AC5: palloc0 (mcxt.c:1118) ==1805451==by 0x1E0A07: index_form_tuple (indextuple.c:146) ==1805451==by 0x210BA8: gistFormTuple (gistutil.c:582) ==1805451==by 0x2084C2: gistBuildCallback (gistbuild.c:813) ==1805451==by 0x23B572: heapam_index_build_range_scan (heapam_handler.c:1694) ==1805451==by 0x208E7D: table_index_build_scan (tableam.h:1756) ==1805451==by 0x208E7D: gistbuild (gistbuild.c:309) ==1805451==by 0x2D10C8: index_build (index.c:2983) ==1805451==by 0x2D2A7D: index_create (index.c:1232) ==1805451==by 0x383E67: DefineIndex (indexcmds.c:1128) ==1805451==by 0x5AED2E: ProcessUtilitySlow (utility.c:1535) ==1805451==by 0x5AE262: standard_ProcessUtility (utility.c:1066) ==1805451==by 0x5AE33A: ProcessUtility (utility.c:527) ==1805451== ==1805451== VALGRINDERROR-END regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink=2021-11-06%2023%3A56%3A57
Re: GiST operator class for bool
On 11/3/21 16:18, Tomas Vondra wrote: Hi, I looked at this patch today - it's pretty simple and in pretty good shape, I can't think of anything that'd need fixing. Perhaps the test might also do EXPLAIN like for other types, to verify the new index is actually used. But that's minor enough to handle during commit. I've marked this as RFC and will get it committed in a day or two. Pushed, after adding some simple EXPLAIN to the regression test. Thanks for the patch! regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: GiST operator class for bool
Hi, I looked at this patch today - it's pretty simple and in pretty good shape, I can't think of anything that'd need fixing. Perhaps the test might also do EXPLAIN like for other types, to verify the new index is actually used. But that's minor enough to handle during commit. I've marked this as RFC and will get it committed in a day or two. -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: GiST operator class for bool
> 8 июня 2021 г., в 19:53, Emre Hasegeli написал(а): > >> But patch that you propose does not support sorting build added in PG14. > > It looks like the change to btree_gist is not committed yet. I'll > rebase my patch once it's committed. Changes to GiST are committed. There will be no need to rebase anyway :) > > It was a long thread. I couldn't read all of it. Though, the last > patches felt to me like a part of what's already been committed. > Shouldn't they also be committed to version 14? Well, yeah, it could would be cool to have gist build and gist_btree support it together, but there was many parts and we could did not finish it before feature freeze. Best regards, Andrey Borodin.
Re: GiST operator class for bool
> But patch that you propose does not support sorting build added in PG14. It looks like the change to btree_gist is not committed yet. I'll rebase my patch once it's committed. It was a long thread. I couldn't read all of it. Though, the last patches felt to me like a part of what's already been committed. Shouldn't they also be committed to version 14?
Re: GiST operator class for bool
Hi! > 8 июня 2021 г., в 13:48, Emre Hasegeli написал(а): > > It could be useful to use bool in exclusion constraints, but it's > currently not nicely supported. The attached patch adds support for > bool to the btree_gist extension, so we can do this. > > I am adding this to the commitfest 2021-07. > <0001-btree_gist-Support-bool.patch> It definitely makes sense to include bool into list of supported types. But patch that you propose does not support sorting build added in PG14. Or we can add this functionality later in https://commitfest.postgresql.org/31/2824/ ... Thanks! Best regards, Andrey Borodin.
GiST operator class for bool
It could be useful to use bool in exclusion constraints, but it's currently not nicely supported. The attached patch adds support for bool to the btree_gist extension, so we can do this. I am adding this to the commitfest 2021-07. 0001-btree_gist-Support-bool.patch Description: Binary data
