POC: Unlocked path for GetSnapshotDataReuse

Hi,

GetSnapshotData() takes the ProcArrayLock in read-only mode for several reasons:

1. To acquire new snapshot data;
2. To make sure that no active transaction commits before we install
the new snapshot data's xmin, if we didn't already have an xmin
installed; and
3. To validate that the snapshot it currently holds hasn't been
invalidated by a newly committed transaction, by comparing
snapshot->transXactCompletionCount against
TransamVariables->xactCompletionCount.

I noticed [0]after implementing this, I looked at commit logs for the Reuse path, it looks like the original xactCompletionCount commit 623a9ba79b already hinted at this optimization in its message as being a likely possible further optimization. However, nobody seems to have since gotten to actually implementing it. that the reuse path could be implemented as just an
atomic read (thus avoiding touching the cache line that backs
ProcArrayLock) if these conditions hold:

1. We have a cached snapshot that we'd like to reuse;
2. Our backend already has its MyProc->xmin installed; and
3. The output of an atomic read of
TransamVariables->xactCompletionCount indicates its value hasn't
changed since the to-be-reused snapshot was taken.

Actually updating the snapshot contents would still require taking the
lock, and likewise would installing MyProc->xmin, but for some
workloads this'll probably save a lot of RW traffic on the
ProcArrayLock cache line.

Data collected from running the test suite with some instrumentation
(in 0002-nocfbot.patch) indicates ~50% of GetSnapshotData() calls
benefit from this unlocked GetSnapshotDataReuse optimization: 1174391
of 2288104 calls to GetSnapshotData used the new unlocked path, with
866907 (37%) not taking the path because it would need to install an
xmin, and 246134 (11%) failing on the xactCompletionCount check and
needing to update the snapshot, whilst the remaining 672 (0.03%) of
the sessions didn't have a cached snapshot to reuse.

I've attached a patch that applies this optimization. The patch is
quite a bit larger larger than I'd hoped, because adding
port/atomics.h to access/transam.h causes frontend compilation errors.
This effectively required me to move TransamVariables into a different
header, this case a new varsup.h, which we then need to included in
many sources, which increases the size of the patch.

Thoughts?

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

[0]: after implementing this, I looked at commit logs for the Reuse path, it looks like the original xactCompletionCount commit 623a9ba79b already hinted at this optimization in its message as being a likely possible further optimization. However, nobody seems to have since gotten to actually implementing it.
path, it looks like the original xactCompletionCount commit 623a9ba79b
already hinted at this optimization in its message as being a likely
possible further optimization. However, nobody seems to have since
gotten to actually implementing it.