Re: HOT patch - version 14 - Mailing list pgsql-patches

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> Please see the version 14 of HOT patch attached.

I expected to find either a large new README, or some pretty substantial
additions to existing README files, to document how this all works.
The comments included do not represent nearly enough documentation.

One thing I was unable to discern from the comments is how CREATE INDEX
can work at all.  A new index might mean that tuples that could formerly
legally be part of the same hot-chain no longer can.  I can't find any
code here that looks like it's addressing that.  I also don't think I
believe the reasoning for not indexing DELETE_IN_PROGRESS hot-updated
tuples: what if the index completion commits, but the concurrent delete
rolls back?  Then you've got a valid tuple that's not in the index.

> I also took this opportunity to remove the modularity invasion caused
> by heap_check_idxupdate() since it was using resultRelInfo. We now
> build the list of attributes that must be checked to satisfy HOT update.
> This list includes all the index columns, columns in the partial index
> predicates and expression index expressions and is built in the
> executor.

The executor is the wrong place for that: I'm not sure why you think
that making heapam depend on the executor is a modularity improvement.
Furthermore this approach requires recalculating the list during
each query, which is wasteful when it could only change during schema
updates.  I'd suggest making the relcache responsible for computing and
saving this data, along the same lines as RelationGetIndexList().
(That also means that heapam can get the data from the relcache, saving
a lot of API-bloating from passing around Attrids explicitly.)
Also, rather than inventing Attrids, I'd suggest just using one
Bitmapset with the convention that its contents are offset by
FirstLowInvalidHeapAttributeNumber.

The redefinition of the value of MaxHeapTuplesPerPage seems pretty
ugly and unprincipled.  I think it'd be better to leave it as-is,
and just enforce that we don't allow more than that many line pointers
on a heap page (as indeed you have to do anyway, so it's not clear
what the change is buying).

Is it really necessary to add hot_update to xl_heap_update?  Seems the
information should be available from the tuple header fields.

Have you demonstrated that the "prune_hard" logic is worth its weight?
Considering that many of us want to drop VACUUM FULL, adding more
complexity to it doesn't seem like a profitable use of time.

Is it really safe, or productive, to run heap_page_prune when the buffer
is not locked for cleanup (ie, there are other people with pins on it)?
Even if it's safe, ISTM what you're mostly accomplishing there is to
expend a lot of cycles while holding exclusive lock on the page, when
there is good reason to think that you're blocking other people who are
interested in using the page.  Eliminating the separation between that
and cleanup would also allow eliminating the separate "PD_FRAGMENTED"
page state.

PlanSetValidForThisTransaction is completely bogus --- it's not
re-entrant, and it needs to be.  I think you need some state in
PlannerGlobal instead.

rd_avgfsm seems fairly bogus ... when does it get updated?

            regards, tom lane