Amit Kapila <amit.kapila16@gmail.com> writes:
> On Fri, Aug 21, 2015 at 8:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Aug 19, 2015 at 2:53 AM, Amit Kapila <amit.kapila16@gmail.com>
>>> Another minor point is, I think we should modify function level comment
>>> for XidInMVCCSnapshot() where it says that this applies to known-
>>> committed XIDs which will no longer be true after this patch.
Yeah, that comment would certainly be out of date, and I think it may not
be the only one. I'll check around some more.
> I am wondering that is there any harm in calling TransactionIdDidAbort()
> in slow path before calling SubTransGetTopmostTransaction(), that can
> also maintain consistency of checks in both the functions?
I think this is probably a bad idea. It adds a pg_clog lookup that we
would otherwise not do at all, in hopes of avoiding a pg_subtrans lookup.
It's not exactly clear that that's a win even if we successfully avoid
the subtrans lookup (which we often would not). And even if it does win,
that would only happen if the other transaction has aborted, which isn't
generally the case we prefer to optimize for.
I don't mean to dismiss the potential for further optimization inside
XidInMVCCSnapshot (for instance, the one-XID-cache idea sounds promising);
but I think that's material for further research and a separate patch.
It's not clear to me if anyone wanted to do further review/testing of
this patch, or if I should go ahead and push it (after fixing whatever
comments need to be fixed).
regards, tom lane