Re: Seq scans status update - Mailing list pgsql-patches

Heikki Linnakangas <heikki@enterprisedb.com> writes:
> Here's a new version, all known issues are now fixed. I'm now happy with
> this patch.

I'm looking this over and finding it fairly ugly from a
system-structural point of view.  In particular, this has pushed the
single-global-variable StrategyHintVacuum() concept well past the
breaking point.  That API was sort of tolerable for vacuum, since vacuum
isn't re-entrant and can't coexist with any other behavior within the
same backend; though I never liked the fact that it affected vacuum's
system-catalog accesses along with the intended table fetches.  But now
you've got bits of code hacking the access pattern in contexts that are
far less predictable than vacuum ... and not guaranteeing to reset the
access pattern on failure, either.

I think we've got to get rid of the global variable and make the access
pattern be a parameter to StrategyGetBuffer, instead.  Which in turn
suggests a variant ReadBuffer, maybe ReadBufferWithStrategy(), at the
next level up.  This would serve directly for the heapscan case, and
for the main heap accesses in vacuum/analyze, but it might be a bit
of a pain to make index vacuuming pay attention to the strategy.
The other case that the patch addresses is COPY TO REL, which we could
handle if we were willing to pass a strategy parameter down through
heap_insert and RelationGetBufferForTuple ... is it worth the trouble?
I don't recall anyone having presented measurements to show COPY TO REL
being helped by this patch.

I don't especially care for the ring data structure being global inside
freelist.c, either.  What I'm inclined to do is have the "access
strategy parameter" actually be a pointer to some struct type, with NULL
representing the default strategy.  At the beginning of a bulk operation
we'd create an access strategy object, which would be internally the
current Ring data structure, but it'd be opaque to code outside
freelist.c.  This'd mean that a query using two large seqscans would use
two rings not one, but that's not necessarily bad; it would in any case
make it a lot easier to generalize the strategy concept in future to
support more than one kind of non-default policy being active in
different parts of a query.

A point I have not figured out how to deal with is that in the patch as
given, UnpinBuffer needs to know the strategy; and getting it that info
would make the changes far more invasive.  But the patch's behavior here
is pretty risky anyway, since the strategy global at the time of unpin
might have nothing to do with how it was set when the buffer was
acquired.  What I'm tempted to do is remove the special case there and
adjust buffer acquisition instead (maybe make it decrement the
usage_count when re-using a buffer from the ring).

Comments?  I'm still playing with the ideas at this point...

            regards, tom lane