Re: preserving forensic information when we freeze

On 2013-05-28 21:26:49 -0400, Robert Haas wrote:
> On Tue, May 28, 2013 at 8:00 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > I only suggested MOVED_IN/OFF because I didn't remember
> > HEAP_XMIN_COMMITTED|HEAP_XMIN_INVALID was still free ;). So, unless that
> > combination could have been produced in the past in a way I couldn't
> > find so far, I am all for it. I don't see a problem with breaking
> > backward compatibility on that level and I don't think there's any way
> > to get there without some level of low level compat break.
>
> I'm not sure why you call it a compatibility break. It's true that an
> old PostgreSQL can't read new heaps, but we've never guaranteed that
> direction anyway, and every time we add or reinterpret any infomask
> bits, that's true. For example, the fklocks patch did tat. What's
> important is that a new PostgreSQL will still be able to read old
> heaps; that is, pg_upgrade compatibility is preserved.

Oh, not the on-disk format. But as you said, code that looks at xmin is
going to need to change. fklocks broke code that relied on
HeapTupleHeaderGetXmax(), this would break code that looks at
xmin. Removing/renaming *GetXmin similarly sounds like a good idea to
make the breakage explicit.

> > I am all for adding a comment why this is safe though. We thought about
> > it for some while before we were convinced...
>
> I'm fine with that, but the logic is present in multiple places, and I
> did not want to comment them all identically. If there's a central
> place in which a comment would be appropriate, let's add it there; or
> IOW, what do you suggest in detail?

That's a good point. Generally lots of this is underdocumented/commented
and can only learned by spending a year or so in the postgres code. I'd
say rename README.HOT to README and add a section there and reference it
from those two places? It already has a mostly general explanation of
concurrent index builds. Don't have a better idea.

> > Hm. As previously said, I am less than convinced of those adhoc
> > mechanisms and I think this should get properly integrated into the
> > normal cache invalidation mechanisms.
> > But: I think this is safe since we compare the stored/cached xmin/tid
> > with one gotten from the SearchSysCache just before which will point to
> > the correct location as of the last AcceptInvalidationMessages(). I
> > can't think of a way were this would allow the case you describe.
>
> I don't understand why it can't. AcceptInvalidationMessages()
> guarantees that we're looking at the latest version of the catalog,
> but it doesn't say anything about whether the latest catalog state
> happens to look like any earlier catalog state.

Well, AcceptInvalidationMessages() will get a new version of the tuple
with a new tid/xmin combo. So what would need to happen is the function
being updated (to a new location), then the old version needs to get
removed, then the new one updated again, reusing to the old
location. Allthewhile either both versions need to have been frozen or
we need to have wrapped around to the same xid. All that without the
function being executed inbetween which would have invalidated the old
state and stored a new xmin/tid.
But you're right, nothing except chance prevents that from happening,
not sure what I thought of earlier.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services