From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
To: Laurenz Albe <laurenz.albe@cybertec.at>
To: Tom Lane <tgl@sss.pgh.pa.us>
To: Simon Riggs <simon@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Christoph Berg <myon@debian.org>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Surjective functional indexes
Date: Thu, 8 Nov 2018 17:33:27 +0300
Message-ID: <bbf2c898-de62-68b5-dd22-b5a04520d286@postgrespro.ru> (raw)
In-Reply-To: <a9b352e807b53ef04b307e1453399fd601bdf076.camel@cybertec.at>
References: <aad81201-8788-a621-2001-537d843186fb@postgrespro.ru>
<20171214211553.ijxfd4p52nz7qg44@alvherre.pgsql>
<CAB7nPqR8GPni-Y_Tr1+6XFdYCuum8a5MuB9q2+hnVcA7BYrorw@mail.gmail.com>
<623fdf0f-c3d1-7553-e784-f8d5a900239b@postgrespro.ru>
<20180106225942.GT2416@tamriel.snowman.net>
<40fd1ef1-6227-69d1-9a24-edfef875e006@postgrespro.ru>
<20180301194824.46ivyszju2ivhlh5@alap3.anarazel.de>
<CA+TgmoZzdNsTFmmt+wTtCn0hQ+1F=OEG6ptbSRbi6prqVa97Cg@mail.gmail.com>
<20180511033227.vvgm3yi5xngro2hp@alap3.anarazel.de>
<CANP8+jKD+u-oW-whEdx1+k_NjAwHU4PvNse-qLp-o_d2zSXW7A@mail.gmail.com>
<20180511143737.rvogcsxdksrhyyj5@alap3.anarazel.de>
<CANP8+jLgdS-_ka5bhyG_=f5wGYVY-bQy3YD7rJY=TDnpA1snmg@mail.gmail.com>
<CA+Tgmob_qk-w38Mbp5wCCGH9G8AFpmB0K0aTLujJqHNbBpBNUA@mail.gmail.com>
<25975.1541618754@sss.pgh.pa.us>
<a9b352e807b53ef04b307e1453399fd601bdf076.camel@cybertec.at>
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
Attachments:projection-10.patch (text/x-patch, 10.4K) downloaddiff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgmlindex 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 ] <replaceable class=
<listitem>
<para>
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.cindex 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.cindex 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.cindex 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.hindex 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.hindex 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.outindex 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.sqlindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: k.knizhnik@postgrespro.ru, laurenz.albe@cybertec.at, tgl@sss.pgh.pa.us, simon@2ndquadrant.com, robertmhaas@gmail.com, andres@anarazel.de, sfrost@snowman.net, michael.paquier@gmail.com, alvherre@alvh.no-ip.org, myon@debian.org, thomas.munro@enterprisedb.com, peter.eisentraut@2ndquadrant.com
Subject: Re: [HACKERS] Surjective functional indexes
In-Reply-To: <bbf2c898-de62-68b5-dd22-b5a04520d286@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox