On Wed, 16 Nov 2022 at 15:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 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.
Completely understand. It took me months to think this through.
> 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.
Yes, apologies for that, I focused on the holistic explanation in the slides.
> 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.)
New version with greatly improved comments coming very soon.
> 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.
Yes, I thought you might ask that so, after some thought, have found a
clean way to do that and have split this into two parts.
Julien has agreed to do further perf tests and is working on that now.
I will post new versions soon, earliest tomorrow.
--
Simon Riggs http://www.EnterpriseDB.com/
