Re: Nasty bug in heap_page_prune

"Pavan Deolasee" <pavan(dot)deolasee(at)gmail(dot)com> writes:
> On Thu, Mar 6, 2008 at 6:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> While working on the previously discussed refactoring of
>> heap_page_prune, I came to the realization that its use of
>> CacheInvalidateHeapTuple is not just a PANIC risk but simply wrong :-(
>> The semantics aren't right: inval.c assumes that it's dealing with
>> transactional invalidations,

> I am not sure how ugly or difficult it would be to teach inval.c to handle
> non-transactional invalidations.

After further study I think it might not be that hard. As things stand,
CacheInvalidateHeapTuple accumulates notices into a list that is pushed
out at transaction commit. But we could push them out earlier, ie,
before making the actual page changes in heap_page_prune. This seems
safe since an unnecessary invalidation notice cannot break anything,
at worst it causes useless work.

My first visualization of how to do this was to extend the current
subtransaction handling logic in inval.c, such that we'd build a phony
"subtransaction" for each invocation of heap_page_prune, and then force
the messages to be sent at "subtransaction commit". However, it looks
to me like we don't even need that much mechanism. The problem case
only occurs during the first stage of VACUUM FULL, and AFAICS there
could never be any other pending inval events at that point. (VACUUM
FULL will generate transactional inval events later, when it's doing
cross-page tuple moves, but there shouldn't be any during the time that
we're running heap_page_prune.) So I think the only mechanism we really
need is something to force out pending inval events immediately, that
is just AtEOXact_Inval(true) or something pretty close to it.

I'm inclined to set this up as having heap_page_prune first call
a function named something like "BeginNontransactionalInvalidation",
then do its CacheInvalidateHeapTuple calls, then call
"EndNontransactionalInvalidation". In the initial implementation
the first would just assert that the inval queue is empty, and the
second would push out the queue. If we ever need to generalize
things then the code structure would be there to build a phony
subtransaction.

Obviously this is a bit of a hack, but there's hardly anything about
VACUUM FULL that's not a bit of a hack ...

I haven't coded this, much less tested it, but it seems like it'd
work. Comments?

regards, tom lane