Heikki Linnakangas <[EMAIL PROTECTED]> 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 ---------------------------(end of broadcast)--------------------------- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq