v1-0001-Preliminary-refactoring.patch

From b6c502a8ed215b51dd26179194d9ddb6885f6e13 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
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