From b6c502a8ed215b51dd26179194d9ddb6885f6e13 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 1 Feb 2025 17:59:54 -0500 Subject: [PATCH v1 1/5] Preliminary refactoring. This step doesn't change any behavior. It cleans the code up slightly and documents it better. In particular, the trick 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. --- contrib/btree_gin/btree_gin.c | 85 +++++++++++++++++++------------- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 53 insertions(+), 33 deletions(-) diff --git a/contrib/btree_gin/btree_gin.c b/contrib/btree_gin/btree_gin.c index 533c55e9ea..d364e72226 100644 --- a/contrib/btree_gin/btree_gin.c +++ b/contrib/btree_gin/btree_gin.c @@ -16,14 +16,18 @@ PG_MODULE_MAGIC; +/* 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 @@ -33,6 +37,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; @@ -41,19 +46,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); @@ -62,20 +59,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: @@ -94,14 +100,17 @@ 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) { @@ -111,12 +120,22 @@ gin_btree_compare_prefix(FunctionCallInfo fcinfo) int32 res, cmp; + /* + * The core GIN code only calls this function to compare the original + * comparison datum ("a") to an index entry ("b"). However, what it + * thinks is the original comparison datum is the entry value reported by + * gin_btree_extract_query, which might be different. To get the right + * answers we must compare to the original datum from the query. But + * first, assert that "a" 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 this assumption. + */ + Assert(a == data->entry_datum); + cmp = DatumGetInt32(CallerFInfoFunctionCall2(data->typecmp, fcinfo->flinfo, PG_GET_COLLATION(), - (data->strategy == BTLessStrategyNumber || - data->strategy == BTLessEqualStrategyNumber) - ? data->datum : a, + data->orig_datum, b)); switch (data->strategy) @@ -149,7 +168,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/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index a2644a2e65..cb9388f61a 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3377,6 +3377,7 @@ bloom_filter boolKEY brin_column_state brin_serialize_callback_type +btree_gin_leftmost_function bytea cached_re_str canonicalize_state -- 2.43.5