From 48e6fdaaa20d1c9f9157d36e59a4ce61a91086d7 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 2 Jul 2025 12:08:19 -0400 Subject: [PATCH v5 2/6] Preliminary refactoring. This step doesn't change any behavior. It cleans the code up slightly and documents it better. In particular, the test being used by gin_btree_compare_prefix is better explained (IMO) and there's now an Assert backing up the assumption it has to make. Discussion: https://postgr.es/m/262624.1738460652@sss.pgh.pa.us --- contrib/btree_gin/btree_gin.c | 90 +++++++++++++++++++------------- doc/src/sgml/gin.sgml | 6 ++- src/tools/pgindent/typedefs.list | 1 + 3 files changed, 60 insertions(+), 37 deletions(-) diff --git a/contrib/btree_gin/btree_gin.c b/contrib/btree_gin/btree_gin.c index 98663cb8611..2eac6430b10 100644 --- a/contrib/btree_gin/btree_gin.c +++ b/contrib/btree_gin/btree_gin.c @@ -19,14 +19,18 @@ PG_MODULE_MAGIC_EXT( .version = PG_VERSION ); +/* extra data passed from gin_btree_extract_query to gin_btree_compare_prefix */ typedef struct QueryInfo { - StrategyNumber strategy; - Datum datum; - bool is_varlena; - Datum (*typecmp) (FunctionCallInfo); + StrategyNumber strategy; /* operator strategy number */ + Datum orig_datum; /* original query (comparison) datum */ + Datum entry_datum; /* datum we reported as the entry value */ + PGFunction typecmp; /* appropriate btree comparison function */ } QueryInfo; +typedef Datum (*btree_gin_leftmost_function) (void); + + /*** GIN support functions shared by all datatypes ***/ static Datum @@ -36,6 +40,7 @@ gin_btree_extract_value(FunctionCallInfo fcinfo, bool is_varlena) int32 *nentries = (int32 *) PG_GETARG_POINTER(1); Datum *entries = (Datum *) palloc(sizeof(Datum)); + /* Ensure that values stored in the index are not toasted */ if (is_varlena) datum = PointerGetDatum(PG_DETOAST_DATUM(datum)); entries[0] = datum; @@ -44,19 +49,11 @@ gin_btree_extract_value(FunctionCallInfo fcinfo, bool is_varlena) PG_RETURN_POINTER(entries); } -/* - * For BTGreaterEqualStrategyNumber, BTGreaterStrategyNumber, and - * BTEqualStrategyNumber we want to start the index scan at the - * supplied query datum, and work forward. For BTLessStrategyNumber - * and BTLessEqualStrategyNumber, we need to start at the leftmost - * key, and work forward until the supplied query datum (which must be - * sent along inside the QueryInfo structure). - */ static Datum gin_btree_extract_query(FunctionCallInfo fcinfo, bool is_varlena, - Datum (*leftmostvalue) (void), - Datum (*typecmp) (FunctionCallInfo)) + btree_gin_leftmost_function leftmostvalue, + PGFunction typecmp) { Datum datum = PG_GETARG_DATUM(0); int32 *nentries = (int32 *) PG_GETARG_POINTER(1); @@ -65,20 +62,29 @@ gin_btree_extract_query(FunctionCallInfo fcinfo, Pointer **extra_data = (Pointer **) PG_GETARG_POINTER(4); Datum *entries = (Datum *) palloc(sizeof(Datum)); QueryInfo *data = (QueryInfo *) palloc(sizeof(QueryInfo)); - bool *ptr_partialmatch; + bool *ptr_partialmatch = (bool *) palloc(sizeof(bool)); - *nentries = 1; - ptr_partialmatch = *partialmatch = (bool *) palloc(sizeof(bool)); - *ptr_partialmatch = false; + /* + * Detoast the comparison datum. This isn't necessary for correctness, + * but it can save repeat detoastings within the comparison function. + */ if (is_varlena) datum = PointerGetDatum(PG_DETOAST_DATUM(datum)); - data->strategy = strategy; - data->datum = datum; - data->is_varlena = is_varlena; - data->typecmp = typecmp; - *extra_data = (Pointer *) palloc(sizeof(Pointer)); - **extra_data = (Pointer) data; + /* Prep single comparison key with possible partial-match flag */ + *nentries = 1; + *partialmatch = ptr_partialmatch; + *ptr_partialmatch = false; + + /* + * For BTGreaterEqualStrategyNumber, BTGreaterStrategyNumber, and + * BTEqualStrategyNumber we want to start the index scan at the supplied + * query datum, and work forward. For BTLessStrategyNumber and + * BTLessEqualStrategyNumber, we need to start at the leftmost key, and + * work forward until the supplied query datum (which we'll send along + * inside the QueryInfo structure). Use partial match rules except for + * BTEqualStrategyNumber. + */ switch (strategy) { case BTLessStrategyNumber: @@ -97,30 +103,42 @@ gin_btree_extract_query(FunctionCallInfo fcinfo, elog(ERROR, "unrecognized strategy number: %d", strategy); } + /* Fill "extra" data */ + data->strategy = strategy; + data->orig_datum = datum; + data->entry_datum = entries[0]; + data->typecmp = typecmp; + *extra_data = (Pointer *) palloc(sizeof(Pointer)); + **extra_data = (Pointer) data; + PG_RETURN_POINTER(entries); } -/* - * Datum a is a value from extract_query method and for BTLess* - * strategy it is a left-most value. So, use original datum from QueryInfo - * to decide to stop scanning or not. Datum b is always from index. - */ static Datum gin_btree_compare_prefix(FunctionCallInfo fcinfo) { - Datum a = PG_GETARG_DATUM(0); - Datum b = PG_GETARG_DATUM(1); + Datum partial_key PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_DATUM(0); + Datum key = PG_GETARG_DATUM(1); QueryInfo *data = (QueryInfo *) PG_GETARG_POINTER(3); int32 res, cmp; + /* + * partial_key is only an approximation to the real comparison value, + * especially if it's a leftmost value. We can get an accurate answer by + * doing a possibly-cross-type comparison to the real comparison value. + * But just to be sure that things are what we expect, let's assert that + * partial_key is indeed what gin_btree_extract_query reported, so that + * we'll notice if anyone ever changes the core code in a way that breaks + * our assumptions. + */ + Assert(partial_key == data->entry_datum); + cmp = DatumGetInt32(CallerFInfoFunctionCall2(data->typecmp, fcinfo->flinfo, PG_GET_COLLATION(), - (data->strategy == BTLessStrategyNumber || - data->strategy == BTLessEqualStrategyNumber) - ? data->datum : a, - b)); + data->orig_datum, + key)); switch (data->strategy) { @@ -152,7 +170,7 @@ gin_btree_compare_prefix(FunctionCallInfo fcinfo) res = 1; break; case BTGreaterStrategyNumber: - /* If original datum <= indexed one then return match */ + /* If original datum < indexed one then return match */ /* If original datum == indexed one then continue scan */ if (cmp < 0) res = 0; diff --git a/doc/src/sgml/gin.sgml b/doc/src/sgml/gin.sgml index 46e87e01324..82410b1fbdf 100644 --- a/doc/src/sgml/gin.sgml +++ b/doc/src/sgml/gin.sgml @@ -394,7 +394,11 @@ Pointer extra_data) - Compare a partial-match query key to an index key. Returns an integer + Compare a partial-match query key to an index key. + partial_key is a query key that was returned + by extractQuery with an indication that it + requires partial match, and key is an index entry. + Returns an integer whose sign indicates the result: less than zero means the index key does not match the query, but the index scan should continue; zero means that the index key does match the query; greater than zero diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 220e5a4f6b3..2b37a1c74c0 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3475,6 +3475,7 @@ bloom_filter boolKEY brin_column_state brin_serialize_callback_type +btree_gin_leftmost_function bytea cached_re_str canonicalize_state -- 2.43.5