From: | Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Julien Tachoires <julmon(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Japin Li <japinli(at)hotmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: SUBTRANS: Minimizing calls to SubTransSetParent() |
Date: | 2022-11-17 17:04:39 |
Message-ID: | CANbhV-F5j1Q7WYoa2orULJARLCHHFdccwmYMnP=CjaRZ8tKwaA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, 16 Nov 2022 at 15:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com> writes:
> > On Wed, 16 Nov 2022 at 00:09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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/
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2022-11-17 17:07:16 | Re: logical decoding and replication of sequences, take 2 |
Previous Message | Gilles Darold | 2022-11-17 17:03:55 | Re: [BUG] pg_dump blocked |