Re: SUBTRANS: Minimizing calls to SubTransSetParent()

On Wed, 16 Nov 2022 at 00:09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com> writes:
> > On Tue, 15 Nov 2022 at 21:03, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> The subxidStatus.overflowed check quoted above has a similar sort
> >> of myopia: it's checking whether our current transaction has
> >> already suboverflowed. But (a) that doesn't prove it won't suboverflow
> >> later, and (b) the relevant logic in XidInMVCCSnapshot needs to run
> >> SubTransGetTopmostTransaction if *any* proc in the snapshot has
> >> suboverflowed.
>
> > Not the way it is coded now.
>
> > First, we search the subxid cache in snapshot->subxip.
> > Then, and only if the snapshot overflowed (i.e. ANY xact overflowed),
> > do we check subtrans.
>
> 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.

> We could perhaps make it do what you suggest,

Already done in the patch since v5.

> but that would require
> a complete performance analysis to make sure we're not giving up
> more than we would gain.

I agree that a full performance analysis is sensible and an objective
analysis has been performed by Julien Tachoires. This is described
here, along with other explanations:
https://docs.google.com/presentation/d/1A7Ar8_LM5EdC2OHL_j3U9J-QwjMiGw9mmXeBLJOmFlg/edit?usp=sharing

It is important to understand the context here: there is already a
well documented LOSS of performance with the current coding. The patch
alleviates that, and I have not been able to find a performance case
where there is any negative impact.

Further tests welcome.

> Also, both GetSnapshotData and CopySnapshot assume that the subxips
> array is not used if suboverflowed is set, and don't bother
> (continuing to) populate it. So we would need code changes and
> additional cycles in those areas too.

Already done in the patch since v5.

Any additional cycles apply only to the case of snapshot overflow,
which currently performs very badly.

> I'm not sure about your other claims, but I'm pretty sure this one
> point is enough to kill the patch.

Then please look again because there are misunderstandings above.

--
Simon Riggs http://www.EnterpriseDB.com/