Horton

Re: Improving connection scalability: GetSnapshotData()

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: David Rowley <dgrowleyml@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-03-29T00:49:20Z
Lists: pgsql-hackers 
Hi,

On 2020-03-17 23:59:14 +1300, David Rowley wrote:
> Nice performance gains.

Thanks.


> On Sun, 1 Mar 2020 at 21:36, Andres Freund <andres@anarazel.de> wrote:
> 2. I don't quite understand your change in
> UpdateSubscriptionRelState(). snap seems unused. Drilling down into
> SearchSysCacheCopy2, in SearchCatCacheMiss() the systable_beginscan()
> passes a NULL snapshot.
> 
> the whole patch does this. I guess I don't understand why 0002 does this.

See the thread at https://postgr.es/m/20200229052459.wzhqnbhrriezg4v2%40alap3.anarazel.de

Basically, the way catalog snapshots are handled right now, it's not
correct to much without a snapshot held. Any concurrent invalidation can
cause the catalog snapshot to be released, which can reset the backend's
xmin. Which in turn can allow for pruning etc to remove required data.

This is part of this series only because I felt I needed to add stronger
asserts to be confident in what's happening. And they started to trigger
all over :( - and weren't related to the patchset :(.


> 4. Did you consider the location of 'delayChkpt' in PGPROC. Couldn't
> you slot it in somewhere it would fit in existing padding?
> 
> 0007

Hm, maybe. I'm not sure what the best thing to do here is - there's some
arguments to be made that we should keep the fields moved from PGXACT
together on their own cacheline. Compared to some of the other stuff in
PGPROC they're still accessed from other backends fairly frequently.


> 5. GinPageIsRecyclable() has no comments at all. I know that
> ginvacuum.c is not exactly the modal citizen for function header
> comments, but likely this patch is no good reason to continue the
> trend.

Well, I basically just moved the code from the macro of the same
name... I'll add something.


> 9. I get the idea you don't intend to keep the debug message in
> InvisibleToEveryoneTestFullXid(), but if you do, then shouldn't it be
> using UINT64_FORMAT?

Yea, I don't intend to keep them - they're way too verbose, even for
DEBUG*. Note that there's some advantage in the long long cast approach
- it's easier to deal with for translations IIRC.

> 13. is -> are
> 
> * accessed data is stored contiguously in memory in as few cache lines as

Oh? 'data are stored' sounds wrong to me, somehow.

Greetings,

Andres Freund

Commits

  1. Try to unbreak 021_row_visibility.pl on mingw.

  2. Fix and test snapshot behavior on standby.

  3. Fix race condition in snapshot caching when 2PC is used.

  4. snapshot scalability: cache snapshots using a xact completion counter.

  5. Fix use of wrong index in ComputeXidHorizons().

  6. Make vacuum a bit more verbose to debug BF failure.

  7. snapshot scalability: Introduce dense array of in-progress xids.

  8. snapshot scalability: Move PGXACT->vacuumFlags to ProcGlobal->vacuumFlags.

  9. snapshot scalability: Move subxact info to ProcGlobal, remove PGXACT.

  10. snapshot scalability: Move PGXACT->xmin back to PGPROC.

  11. snapshot scalability: Don't compute global horizons while building snapshots.

  12. BRIN: Handle concurrent desummarization properly

  13. Track latest completed xid as a FullTransactionId.

  14. Rename VariableCacheData.nextFullXid to nextXid.

  15. snapshot scalability: Move delayChkpt from PGXACT to PGPROC.

  16. Report progress of CREATE INDEX operations