Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1gKlNL-0001CF-Pk for pgsql-hackers@arkaria.postgresql.org; Thu, 08 Nov 2018 14:33:40 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1gKlNJ-0004Pe-Vs for pgsql-hackers@arkaria.postgresql.org; Thu, 08 Nov 2018 14:33:37 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1gKlNJ-0004PX-ED for pgsql-hackers@lists.postgresql.org; Thu, 08 Nov 2018 14:33:37 +0000 Received: from mail.postgrespro.ru ([93.174.131.138]) by makus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1gKlND-0001j8-2Q for pgsql-hackers@postgresql.org; Thu, 08 Nov 2018 14:33:35 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id C1D6F21C4D41; Thu, 8 Nov 2018 17:33:27 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=x tagged_above=-99 required=4 WHITELISTED tests=[] autolearn=unavailable Received: from [192.168.27.200] (gw.postgrespro.ru [93.174.131.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mail.postgrespro.ru (Postfix) with ESMTPSA id 63C7521C4D40; Thu, 8 Nov 2018 17:33:27 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1541687607; bh=Tja0tuimiRwvN9aTxHzWDd3QsMBz17ziXSXkE8yt1mg=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=WF6557KdopcniDT+qGDhFGaJT38bgUJwjAFA8KqnVyFPRvpKPv6L1a7Y6BUxR/5Qm sa+uasgSNULFcWRNTy3x+WyWc1S8WBEis95waBD0Pxu7mSBvRneglNCRSzpFrrVXQV fwvqvyrwLpbFZHOb/fRC/M2Mu1s83xWolQQgJLmI= Subject: Re: [HACKERS] Surjective functional indexes To: Laurenz Albe , Tom Lane , Simon Riggs Cc: Robert Haas , Andres Freund , Stephen Frost , Michael Paquier , Alvaro Herrera , Christoph Berg , Thomas Munro , Peter Eisentraut , PostgreSQL Hackers References: <20171214211553.ijxfd4p52nz7qg44@alvherre.pgsql> <623fdf0f-c3d1-7553-e784-f8d5a900239b@postgrespro.ru> <20180106225942.GT2416@tamriel.snowman.net> <40fd1ef1-6227-69d1-9a24-edfef875e006@postgrespro.ru> <20180301194824.46ivyszju2ivhlh5@alap3.anarazel.de> <20180511033227.vvgm3yi5xngro2hp@alap3.anarazel.de> <20180511143737.rvogcsxdksrhyyj5@alap3.anarazel.de> <25975.1541618754@sss.pgh.pa.us> From: Konstantin Knizhnik Message-ID: Date: Thu, 8 Nov 2018 17:33:27 +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: Content-Type: multipart/mixed; boundary="------------D0D2A365B77F5DC736EAF2E1" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------D0D2A365B77F5DC736EAF2E1 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 08.11.2018 15:23, Laurenz Albe wrote: > Tom Lane wrote: >> I wanted to enumerate my concerns while yesterday's >> events are still fresh in mind. (Andres or Robert might have more.) >> >> * I do not understand why this feature is on-by-default in the first >> place. It can only be a win for expression indexes that are many-to-one >> mappings; for indexes that are one-to-one or few-to-one, it's a pretty >> big loss. I see no reason to assume that most expression indexes fall >> into the first category. I suggest that the design ought to be to use >> this optimization only for indexes for which the user has explicitly >> enabled recheck_on_update. That would allow getting rid of the cost check >> in IsProjectionFunctionalIndex, about which we'd otherwise have to have >> an additional fight: I do not like its ad-hoc-ness, nor the modularity >> violation (and potential circularity) involved in having the relcache call >> cost_qual_eval. > That was my impression too when I had a closer look at this feature. > > What about an option "hot_update_check" with values "off" (default), > "on" and "always"? > > Yours, > Laurenz Albe > 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). -- Konstantin Knizhnik Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --------------D0D2A365B77F5DC736EAF2E1 Content-Type: text/x-patch; name="projection-10.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="projection-10.patch" diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 4df3d75..e17dd09 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -365,8 +365,9 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] Specifies whether to recheck a functional index value to see whether - we can use a HOT update or not. The default value is on for functional - indexes with an total expression cost less than 1000, otherwise off. + we can use a HOT update or not. Accepted values are "on", "auto" and "off" + (default). In case of "auto", recheck is done for indexes with an total expression + cost less than 1000. You might decide to turn this off if you knew that a function used in an index is unlikely to return the same value when one of the input columns is updated and so the recheck is not worth the additional cost diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index db84da0..9fb5ebc 100644 --- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -19,6 +19,7 @@ #include "access/gist_private.h" #include "access/hash.h" +#include "access/heapam.h" #include "access/htup_details.h" #include "access/nbtree.h" #include "access/reloptions.h" @@ -131,15 +132,6 @@ static relopt_bool boolRelOpts[] = }, { { - "recheck_on_update", - "Recheck functional index expression for changed value after update", - RELOPT_KIND_INDEX, - ShareUpdateExclusiveLock /* since only applies to later UPDATEs */ - }, - true - }, - { - { "security_barrier", "View acts as a row security barrier", RELOPT_KIND_VIEW, @@ -448,6 +440,18 @@ static relopt_string stringRelOpts[] = validateWithCheckOption, NULL }, + { + { + "recheck_on_update", + "Recheck functional index expression for changed value after update", + RELOPT_KIND_INDEX, + ShareUpdateExclusiveLock /* since only applies to later UPDATEs */ + }, + 3, + false, + validateRecheckOnUpdateOption, + "off" + }, /* list terminator */ {{NULL}} }; @@ -1499,7 +1503,7 @@ index_generic_reloptions(Datum reloptions, bool validate) GenericIndexOpts *idxopts; relopt_value *options; static const relopt_parse_elt tab[] = { - {"recheck_on_update", RELOPT_TYPE_BOOL, offsetof(GenericIndexOpts, recheck_on_update)} + {"recheck_on_update", RELOPT_TYPE_STRING, offsetof(GenericIndexOpts, recheck_on_update)} }; options = parseRelOptions(reloptions, validate, diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index fb63471..21cdc1b 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -4491,6 +4491,21 @@ heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum, } } + +void validateRecheckOnUpdateOption(const char *value) +{ + if (value == NULL || + (strcmp(value, "on") != 0 && + strcmp(value, "off") != 0 && + strcmp(value, "auto") != 0)) + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid value for \"recheck_on_update\" option"), + errdetail("Valid values are \"on\", \"off\", and \"auto\"."))); + } +} + /* * Check whether the value is unchanged after update of a projection * functional index. Compare the new and old values of the indexed @@ -4546,9 +4561,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..adbdc8a 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -4762,24 +4762,6 @@ IsProjectionFunctionalIndex(Relation index, IndexInfo *ii) bool isnull; 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)); @@ -4794,7 +4776,26 @@ IsProjectionFunctionalIndex(Relation index, IndexInfo *ii) if (idxopts != NULL) { - is_projection = idxopts->recheck_on_update; + char* recheck_on_update = (char*)idxopts + idxopts->recheck_on_update; + if (strcmp(recheck_on_update, "on") == 0) + { + is_projection = true; + } + else if (strcmp(recheck_on_update, "auto") == 0) + { + 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; + } pfree(idxopts); } } diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 40e153f..4c6ecc3 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -201,4 +201,6 @@ extern BlockNumber ss_get_location(Relation rel, BlockNumber relnblocks); extern void SyncScanShmemInit(void); extern Size SyncScanShmemSize(void); +extern void validateRecheckOnUpdateOption(const char *value); + #endif /* HEAPAM_H */ diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 84469f5..b87969a 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -224,7 +224,7 @@ typedef struct ForeignKeyCacheInfo typedef struct GenericIndexOpts { int32 vl_len_; - bool recheck_on_update; + int recheck_on_update; } GenericIndexOpts; /* diff --git a/src/test/regress/expected/func_index.out b/src/test/regress/expected/func_index.out index 307ac97..1d9f2ce 100644 --- a/src/test/regress/expected/func_index.out +++ b/src/test/regress/expected/func_index.out @@ -1,6 +1,6 @@ begin; create table keyvalue(id integer primary key, info jsonb); -create index nameindex on keyvalue((info->>'name')) with (recheck_on_update=false); +create index nameindex on keyvalue((info->>'name')) with (recheck_on_update='off'); insert into keyvalue values (1, '{"name": "john", "data": "some data"}'); update keyvalue set info='{"name": "john", "data": "some other data"}' where id=1; select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); @@ -12,7 +12,7 @@ select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); rollback; begin; create table keyvalue(id integer primary key, info jsonb); -create index nameindex on keyvalue((info->>'name')) with (recheck_on_update=true); +create index nameindex on keyvalue((info->>'name')) with (recheck_on_update='on'); insert into keyvalue values (1, '{"name": "john", "data": "some data"}'); update keyvalue set info='{"name": "john", "data": "some other data"}' where id=1; select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); @@ -38,7 +38,7 @@ select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); rollback; begin; create table keyvalue(id integer primary key, info jsonb); -create index nameindex on keyvalue((info->>'name')); +create index nameindex on keyvalue((info->>'name')) with (recheck_on_update='auto'); insert into keyvalue values (1, '{"name": "john", "data": "some data"}'); update keyvalue set info='{"name": "john", "data": "some other data"}' where id=1; select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); diff --git a/src/test/regress/sql/func_index.sql b/src/test/regress/sql/func_index.sql index c267c93..cb140af 100644 --- a/src/test/regress/sql/func_index.sql +++ b/src/test/regress/sql/func_index.sql @@ -1,6 +1,6 @@ begin; create table keyvalue(id integer primary key, info jsonb); -create index nameindex on keyvalue((info->>'name')) with (recheck_on_update=false); +create index nameindex on keyvalue((info->>'name')) with (recheck_on_update='off'); insert into keyvalue values (1, '{"name": "john", "data": "some data"}'); update keyvalue set info='{"name": "john", "data": "some other data"}' where id=1; select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); @@ -8,7 +8,7 @@ rollback; begin; create table keyvalue(id integer primary key, info jsonb); -create index nameindex on keyvalue((info->>'name')) with (recheck_on_update=true); +create index nameindex on keyvalue((info->>'name')) with (recheck_on_update='on'); insert into keyvalue values (1, '{"name": "john", "data": "some data"}'); update keyvalue set info='{"name": "john", "data": "some other data"}' where id=1; select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); @@ -20,7 +20,7 @@ rollback; begin; create table keyvalue(id integer primary key, info jsonb); -create index nameindex on keyvalue((info->>'name')); +create index nameindex on keyvalue((info->>'name')) with (recheck_on_update='auto'); insert into keyvalue values (1, '{"name": "john", "data": "some data"}'); update keyvalue set info='{"name": "john", "data": "some other data"}' where id=1; select pg_stat_get_xact_tuples_hot_updated('keyvalue'::regclass); --------------D0D2A365B77F5DC736EAF2E1--