Simon Riggs <simon.riggs@enterprisedb.com> writes:
> On Wed, 16 Nov 2022 at 00:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> No, that's not what XidInMVCCSnapshot does. If snapshot->suboverflowed
>> is set (ie, somebody somewhere/somewhen overflowed), then it does
>> SubTransGetTopmostTransaction and searches only the xips with the result.
>> This behavior requires that all live subxids be correctly mapped by
>> SubTransGetTopmostTransaction, or we'll draw false conclusions.
> Your comments are correct wrt to the existing coding, but not to the
> patch, which is coded as described and does not suffer those issues.
Ah, OK.
Still ... I really do not like this patch. It introduces a number of
extremely fragile assumptions, and I think those would come back to
bite us someday, even if they hold now which I'm still unsure about.
It doesn't help that you've chosen to document them only at the place
making them and not at the place(s) likely to break them.
Also, to be blunt, this is not Ready For Committer. It's more WIP,
because even if the code is okay there are comments all over the system
that you've invalidated. (At the very least, the file header comments
in subtrans.c and the comments in struct SnapshotData need work; I've
not looked hard but I'm sure there are more places with comments
bearing on these data structures.)
Perhaps it would be a good idea to split up the patch. The business
about making pg_subtrans flat rather than a tree seems like a good
idea in any event, although as I said it doesn't seem like we've got
a fleshed-out version of that here. We could push forward on getting
that done and then separately consider the rest of it.
regards, tom lane