Re: Improving connection scalability: GetSnapshotData()

Thomas Munro <thomas.munro@gmail.com>

From: Thomas Munro <thomas.munro@gmail.com>

To: Andres Freund <andres@anarazel.de>

Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Daniel Gustafsson <daniel@yesql.se>, Robert Haas <robertmhaas@gmail.com>, "Jonathan S. Katz" <jkatz@postgresql.org>, Alexander Korotkov <a.korotkov@postgrespro.ru>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Peter Geoghegan <pg@bowt.ie>, Bruce Momjian <bruce@momjian.us>, David Rowley <dgrowleyml@gmail.com>

Date: 2020-07-29T06:15:30Z

Lists: pgsql-hackers

On Fri, Jul 24, 2020 at 1:11 PM Andres Freund <andres@anarazel.de> wrote:
> On 2020-07-15 21:33:06 -0400, Alvaro Herrera wrote:
> > On 2020-Jul-15, Andres Freund wrote:
> > > It could make sense to split the conversion of
> > > VariableCacheData->latestCompletedXid to FullTransactionId out from 0001
> > > into is own commit. Not sure...
> >
> > +1, the commit is large enough and that change can be had in advance.
>
> I've done that in the attached.

+     * pair with the memory barrier below.  We do however accept xid to be <=
+     * to next_xid, instead of just <, as xid could be from the procarray,
+     * before we see the updated nextFullXid value.

Tricky.  Right, that makes sense.  I like the range assertion.

+static inline FullTransactionId
+FullXidViaRelative(FullTransactionId rel, TransactionId xid)

I'm struggling to find a better word for this than "relative".

+    return FullTransactionIdFromU64(U64FromFullTransactionId(rel)
+                                    + (int32) (xid - rel_xid));

I like your branch-free code for this.

> I wonder if somebody has an opinion on renaming latestCompletedXid to
> latestCompletedFullXid. That's the pattern we already had (cf
> nextFullXid), but it also leads to pretty long lines and quite a few
> comment etc changes.
>
> I'm somewhat inclined to remove the "Full" out of the variable, and to
> also do that for nextFullXid. I feel like including it in the variable
> name is basically a poor copy of the (also not great) C type system.  If
> we hadn't made FullTransactionId a struct I'd see it differently (and
> thus incompatible with TransactionId), but we have ...

Yeah, I'm OK with dropping the "Full".  I've found it rather clumsy too.

Commits

Try to unbreak 021_row_visibility.pl on mingw.
- 1df2b50dbebb 14.0 landed
Fix and test snapshot behavior on standby.
- 7b28913bcab8 14.0 landed
Fix race condition in snapshot caching when 2PC is used.
- 07f32fcd23ac 14.0 cited
snapshot scalability: cache snapshots using a xact completion counter.
- 623a9ba79bbd 14.0 landed
Fix use of wrong index in ComputeXidHorizons().
- f6661d3df228 14.0 landed
Make vacuum a bit more verbose to debug BF failure.
- 49967da65aec 14.0 landed
snapshot scalability: Introduce dense array of in-progress xids.
- 941697c3c1ae 14.0 landed
snapshot scalability: Move PGXACT->vacuumFlags to ProcGlobal->vacuumFlags.
- 5788e258bb26 14.0 landed
snapshot scalability: Move subxact info to ProcGlobal, remove PGXACT.
- 73487a60fc10 14.0 landed
snapshot scalability: Move PGXACT->xmin back to PGPROC.
- 1f51c17c68d0 14.0 landed
snapshot scalability: Don't compute global horizons while building snapshots.
- dc7420c2c927 14.0 landed
BRIN: Handle concurrent desummarization properly
- 1f42d35a1d61 14.0 cited
Track latest completed xid as a FullTransactionId.
- 3bd7f9969a24 14.0 landed
Rename VariableCacheData.nextFullXid to nextXid.
- fea10a64340e 14.0 landed
snapshot scalability: Move delayChkpt from PGXACT to PGPROC.
- 75848bc74411 13.0 landed
Report progress of CREATE INDEX operations
- ab0dfc961b6a 12.0 cited