Re: [HACKERS] Surjective functional indexes

On 07.11.2018 22:25, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, May 11, 2018 at 11:01 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>>> I have no problem if you want to replace this with an even better
>>>>> design in a later release.
>>>> Meh. The author / committer should get a patch into the right shape
>>> They have done, at length. Claiming otherwise is just trash talk.
>> Some people might call it "honest disagreement".
> So where we are today is that this patch was lobotomized by commits
> 77366d90f/d06fe6ce2 as a result of this bug report:
>
> https://postgr.es/m/20181106185255.776mstcyehnc63ty@alvherre.pgsql
>
> We need to move forward, either by undertaking a more extensive
> clean-out, or by finding a path to a version of the code that is
> satisfactory. 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.
>
> * Having heapam.c calling the executor also seems like a
> modularity/layering violation that will bite us in the future.
>
> * The implementation seems incredibly inefficient. ProjIndexIsUnchanged
> is doing creation/destruction of an EState, index_open/index_close
> (including acquisition of a lock we should already have), BuildIndexInfo,
> and expression compilation, all of which are completely redundant with
> work done elsewhere in the executor. And it's doing that *over again for
> every tuple*, which totally aside from wasted cycles probably means there
> are large query-lifespan memory leaks in an UPDATE affecting many rows.
> I think a minimum expectation should be that one-time work is done only
> one time; but ideally none of those things would happen at all because
> we could share work with the regular code path. Perhaps it's too much
> to hope that we could also avoid duplicate computation of the new index
> expression values, but as long as I'm listing complaints ...
>
> * As noted in the bug thread, the implementation of the new reloption
> is broken because (a) it fails to work for some built-in index AMs
> and (b) by design, it can't work for add-on AMs. I agree with Andres
> that that's not acceptable.
>
> * This seems like bad data structure design:
>
> + Bitmapset *rd_projidx; /* Oids of projection indexes */
>
> That comment is a lie, although IMO it'd be better if it were true;
> a list of target index OIDs would be a far better design here. The use
> of rd_projidx as a set of indexes into the relation's indexlist is
> inefficient and overcomplicated, plus it creates an unnecessary and not
> very safe (even if it were documented) coupling between rd_indexlist and
> rd_projidx.
>
> * Having ProjIndexIsUnchanged examine relation->rd_projidx directly is
> broken by design anyway, both from a modularity standpoint and because
> its inner loop involves catalog accesses that could result in relcache
> flushes. I'm somewhat amazed that the regression tests passed on
> CLOBBER_CACHE_ALWAYS buildfarm animals, although possibly this is
> explained by the fact that they're only testing cases with a single
> expression index, so that the bitmap isn't checked again after the cache
> flush happens. Again, this could be fixed if what was returned was just
> a list of relevant index OIDs.
>
> * This bit of coding is unsafe, for the reason explained in the existing
> comment:
>
> /*
> * Now save copies of the bitmaps in the relcache entry. We intentionally
> * set rd_indexattr last, because that's the one that signals validity of
> * the values; if we run out of memory before making that copy, we won't
> * leave the relcache entry looking like the other ones are valid but
> * empty.
> */
> oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
> relation->rd_keyattr = bms_copy(uindexattrs);
> relation->rd_pkattr = bms_copy(pkindexattrs);
> relation->rd_idattr = bms_copy(idindexattrs);
> relation->rd_indexattr = bms_copy(indexattrs);
> + relation->rd_projindexattr = bms_copy(projindexattrs);
> + relation->rd_projidx = bms_copy(projindexes);
> MemoryContextSwitchTo(oldcxt);
>
> * There's a lot of other inattention to comments. For example, I noticed
> that this patch added new responsibilities to RelationGetIndexList without
> updating its header comment to mention them.
>
> * There's a lack of attention to terminology, too. I do not think that
> "projection index" is an appropriate term at all, nor do I think that
> "recheck_on_update" is a particularly helpful option name, though we
> may be stuck with the latter at this point.
>
> * I also got annoyed by minor sloppiness like not adding the new
> regression test in the same place in serial_schedule and
> parallel_schedule. The whole thing needed more careful review
> than it got.
>
> In short, it seems likely to me that large parts of this patch need to
> be pulled out, rewritten, and then put back in different places than
> they are today. I'm not sure if a complete revert is the best next
> step, or if we can make progress without that.
>
> regards, tom lane
First of all I am sorry for this bug. It's a pity that I was not
involved in this investigation: I am not subscribed on
"postgresql-general" list.
I have found the same bug in yet another my code few month ago. Shame on
me, but I  didn't realized that values returned by FormIndexDatum
data doesn't match with descriptor of index relation, because AM may
convert them to own types. By the way, I will be please if somebody can
suggest the best
way of building proper tuple descriptor.

If the idea of "projection" index optimization is still considered as
valuable by community, I can continue improvement of this patch.
Tom, thank you for the critics.But if it's not too brazen of me, I'd
like to receive more constructive critics:

> Having heapam.c calling the executor also seems like a
modularity/layering violation that will bite us in the future.
Function ProjIndexIsUnchanged is implemented and used in heapam.c
because this is the place where decision weather to perform or no
perform hot update is made.
Insertion of index is done later. So the only possibility is to somehow
cache results of this operation to be able to reuse in future.

> The implementation seems incredibly inefficient. ProjIndexIsUnchanged
is doing creation/destruction of an EState, index_open/index_close...

Well, please look for example at get_actual_variable_range function. It
is doing almost the same. Is it also extremely inefficient?
Almost all functions dealing with indexes and calling FormIndexDatum are
performing the same actions. But the main differences, is that most of
them (like IndexCheckExclusion) initialize EState and create slot only
once and then iterate through all tuples matching search condition. And
ProjIndexIsUnchanged is doing it for each tuple. Once again, I will be
pleased to receive from you (or somebody else) advice how to address
this problem. I have no idea how it it possible to avoid to perform this
check for each affected tuple. So the only solution I can imagine is to
somehow cache and reused created executor state.

> Having ProjIndexIsUnchanged examine relation->rd_projidx directly is
broken by design anyway, both from a modularity standpoint and because
its inner loop involves catalog accesses that could result in relcache
flushes.

Sorry, can you clarify it? As far as there are shared lock for
correspondent relation and index, nobody else can alter this table or
index. And there is rd_refcnt which should prevent destruction of
Relation even if relcache is invalidated. There are a lot of other
places where components of relation are accessed directly (most often
relation->rd_rel stuff). I do not understand why replacing rd_projidx
with list of Oid will solve the problem.

> There's a lack of attention to terminology, too. I do not think that
"projection index" is an appropriate term at all, nor do I think that
"recheck_on_update" is a particularly helpful option name, though we may
be stuck with the latter at this point.

As I am not native english speaking, I will agree with any proposed
terminology.

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