On 28/06/2019 01:02, Thomas Munro wrote:
> On Thu, Jun 27, 2019 at 11:38 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> * I moved the logic to extend a 32-bit XID to 64-bits to a new helper
>> function in varsup.c.
>
> I'm a bit uneasy about making this into a generally available function
> for reuse. I think we should instead come up with a very small number
> of sources of fxids that known to be free of races because of some
> well explained interlocking.
>
> For example, I believe it is safe to convert an xid obtained from a
> WAL record during recovery, because (for now) recovery is a single
> thread of execution and the next fxid can't advance underneath you.
> So I think XLogRecGetFullXid(XLogReaderState *record)[1] as I'm about
> to propose in another thread (though I've forgotten who wrote it,
> maybe Andres, Amit or me, but if it wasn't me then it's exactly what I
> would have written) is a safe blessed source of fxids. The rationale
> for writing this function instead of an earlier code that looked much
> like your proposed helper function, during EDB-internal review of
> zheap stuff, was this: if we provide a general purpose xid->fxid
> facility, it's virtually guaranteed that someone will eventually use
> it to shoot footwards, by acquiring an xid from somewhere, and then
> some arbitrary time later extending it to a fxid when no interlocking
> guarantees that the later conversion has the correct epoch.
Fair point.
> I'd like to figure out how to maintain full versions of the
> procarray.c-managed xid horizons, without widening the shared memory
> representation. I was planning to think harder about for 13.
> Obviously that doesn't help you now. So I'm wondering if you should
> just open-code this for now, and put in a comment about why it's safe
> and a note that there'll hopefully be a fxid horizon available in a
> later release?
I came up with the attached. Instead of having a general purpose "widen"
function, it adds GetFullRecentGlobalXmin(), to return a 64-bit version
of RecentGlobalXmin. That's enough for this Gist vacuum patch.
In addition to that change, I modified the GistPageSetDeleted(),
GistPageSetDeleteXid(), GistPageGetDeleteXid() inline functions a bit. I
merged GistPageSetDeleted() and GistPageSetDeleteXid() into a single
function, to make sure that the delete-XID is always set when a page is
marked as deleted. And I modified GistPageGetDeleteXid() to return
NormalTransactionId (or a FullTransactionId version of it, to be
precise), for Gist pages that were deleted with older PostgreSQL v12
beta versions. The previous patch tripped an assertion in that case,
which is not nice for people binary-upgrading from earlier beta versions.
I did some testing of this with XID wraparound, and it seems to work. I
used the attached bash script for the testing, with the a helper contrib
module to consume XIDs faster. It's not very well commented and probably
not too useful for anyone, but I'm attaching it here mainly as a note to
future-self, if we need to revisit this.
Unless something comes up, I'll commit this tomorrow.
- Heikki