Subject: Re: [HACKERS] Surjective functional indexes
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Laurenz Albe <laurenz.albe@cybertec.at>, Tom Lane <tgl@sss.pgh.pa.us>,
 Simon Riggs <simon@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>,
 Andres Freund <andres@anarazel.de>, Stephen Frost <sfrost@snowman.net>,
 Michael Paquier <michael.paquier@gmail.com>, Christoph Berg
 <myon@debian.org>, Thomas Munro <thomas.munro@enterprisedb.com>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <20181108190559.ylzufqr2jjvml32f@alvherre.pgsql>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <1268584c-cd53-386d-c519-0681e92532c9@postgrespro.ru>
Date: Fri, 9 Nov 2018 20:23:42 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <20181108190559.ylzufqr2jjvml32f@alvherre.pgsql>
Content-Type: multipart/mixed;
 boundary="------------1FEBFE9CE4B60E772C2E568E"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------1FEBFE9CE4B60E772C2E568E
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit


On 08.11.2018 22:05, Alvaro Herrera wrote:
> On 2018-Nov-08, Konstantin Knizhnik wrote:
>
>> Before doing any other refactoring of projection indexes I want to attach
>> small bug fix patch which
>> fixes the original problem (SIGSEGV) and also disables recheck_on_update by
>> default.
>> As Laurenz has suggested, I replaced boolean recheck_on_update option with
>> "on","auto,"off" (default).
> I think this causes an ABI break for GenericIndexOpts.  Not sure to what
> extent that is an actual problem (i.e. how many modules were compiled
> with 11.0 that are gonna be reading that struct with later Pg), but I
> think it should be avoided anyhow.
>
Ok, I reverted back my change of reach_on_update option type.
Now if reach_on_update option is not  explicitly specified, then 
decision is made based on the expression cost.
Patches becomes very small and fix only error in comparison of index 
tuple values.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


--------------1FEBFE9CE4B60E772C2E568E
Content-Type: text/x-patch;
 name="projection-11.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="projection-11.patch"

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index fb63471..81ab0ac 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4546,9 +4546,10 @@ ProjIndexIsUnchanged(Relation relation, HeapTuple oldtup, HeapTuple newtup)
 				}
 				else if (!old_isnull[i])
 				{
-					Form_pg_attribute att = TupleDescAttr(RelationGetDescr(indexDesc), i);
-
-					if (!datumIsEqual(old_values[i], new_values[i], att->attbyval, att->attlen))
+					int16 elmlen;
+					bool elmbyval;
+					get_typlenbyval(indexDesc->rd_opcintype[i], &elmlen, &elmbyval);
+					if (!datumIsEqual(old_values[i], new_values[i], elmbyval, elmlen))
 					{
 						equals = false;
 						break;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index fd3d010..ed8a01e 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4745,10 +4745,11 @@ RelationGetIndexPredicate(Relation relation)
  * for old and new tuple values.
  *
  * Decision made by this function is based on two sources:
- * 1. Calculated cost of index expression: if greater than some heuristic limit
-	  then extra comparison of index expression values is expected to be too
-	  expensive, so we don't attempt it by default.
- * 2. "recheck_on_update" index option explicitly set by user, which overrides 1)
+ * 1. If "recheck_on_update" is true, then index is considered as projection.
+ * 2. If "recheck_on_update" is false, then changing any column on which index depends disables hot update
+ * 3. If "recheck_on_update" is not explicitly specified then recheck on update is
+ *    enabled if cost of index expression is not greater than some heuristic limit (1000).
+ *    In this case extra comparison of index expression values is expected to be too expensive.
  */
 static bool
 IsProjectionFunctionalIndex(Relation index, IndexInfo *ii)
@@ -4760,26 +4761,9 @@ IsProjectionFunctionalIndex(Relation index, IndexInfo *ii)
 		HeapTuple	tuple;
 		Datum		reloptions;
 		bool		isnull;
+		bool		is_explicitly_set = false;
 		QualCost	index_expr_cost;
 
-		/* by default functional index is considered as non-injective */
-		is_projection = true;
-
-		cost_qual_eval(&index_expr_cost, ii->ii_Expressions, NULL);
-
-		/*
-		 * If index expression is too expensive, then disable projection
-		 * optimization, because extra evaluation of index expression is
-		 * expected to be more expensive than index update.  Currently the
-		 * projection optimization has to calculate index expression twice
-		 * when the value of index expression has not changed and three times
-		 * when values differ because the expression is recalculated when
-		 * inserting a new index entry for the changed value.
-		 */
-		if ((index_expr_cost.startup + index_expr_cost.per_tuple) >
-			HEURISTIC_MAX_HOT_RECHECK_EXPR_COST)
-			is_projection = false;
-
 		tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(RelationGetRelid(index)));
 		if (!HeapTupleIsValid(tuple))
 			elog(ERROR, "cache lookup failed for relation %u", RelationGetRelid(index));
@@ -4795,9 +4779,25 @@ IsProjectionFunctionalIndex(Relation index, IndexInfo *ii)
 			if (idxopts != NULL)
 			{
 				is_projection = idxopts->recheck_on_update;
+				is_explicitly_set = true;
 				pfree(idxopts);
 			}
 		}
+		if (!is_explicitly_set)
+		{
+			cost_qual_eval(&index_expr_cost, ii->ii_Expressions, NULL);
+			/*
+			 * If index expression is too expensive, then disable projection
+			 * optimization, because extra evaluation of index expression is
+			 * expected to be more expensive than index update.  Currently the
+			 * projection optimization has to calculate index expression twice
+			 * when the value of index expression has not changed and three times
+			 * when values differ because the expression is recalculated when
+			 * inserting a new index entry for the changed value.
+			 */
+			is_projection = (index_expr_cost.startup + index_expr_cost.per_tuple)
+				<= HEURISTIC_MAX_HOT_RECHECK_EXPR_COST;
+		}
 		ReleaseSysCache(tuple);
 	}
 	return is_projection;

--------------1FEBFE9CE4B60E772C2E568E--