v2-0002-Preliminary-refactoring.patch

From b587446f5e715b56aa3df7c28b621aa86099339b Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri, 7 Feb 2025 17:01:19 -0500
Subject: [PATCH v2 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 533c55e9ea..56383e2786 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,30 +100,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)
 	{
@@ -149,7 +167,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 46e87e0132..82410b1fbd 100644
--- a/doc/src/sgml/gin.sgml
+++ b/doc/src/sgml/gin.sgml
@@ -394,7 +394,11 @@
                               Pointer extra_data)</function></term>
      <listitem>
       <para>
-       Compare a partial-match query key to an index key.  Returns an integer
+       Compare a partial-match query key to an index key.
+       <literal>partial_key</literal> is a query key that was returned
+       by <function>extractQuery</function> with an indication that it
+       requires partial match, and <literal>key</literal> 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 9a3bee93de..e3daab0905 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3379,6 +3379,7 @@ bloom_filter
 boolKEY
 brin_column_state
 brin_serialize_callback_type
+btree_gin_leftmost_function
 bytea
 cached_re_str
 canonicalize_state
-- 
2.43.5