Re: Improving connection scalability: GetSnapshotData() - Mailing list pgsql-hackers

Hi,

Thanks for the review!


On 2020-04-07 12:41:07 -0400, Robert Haas wrote:
> In 0002, the comments in SnapshotSet() are virtually incomprehensible.
> There's no commit message so the reasons for the changes are unclear.
> But mostly looks unproblematic.

I was planning to drop that patch pre-commit, at least for now.  I think
there's a few live bugs here, but they're all older. I did send a few emails
about the class of problem, unfortunately it was a fairly one-sided
conversation so far ;)

https://www.postgresql.org/message-id/20200407072418.ccvnyjbrktyi3rzc%40alap3.anarazel.de


> 0003 looks like a fairly unrelated bug fix that deserves to be
> discussed on the thread related to the original patch. Probably should
> be an open item.

There was some discussion in a separate thread:
https://www.postgresql.org/message-id/20200406025651.fpzdb5yyb7qyhqko%40alap3.anarazel.de
The only reason for including it in this patch stack is that I can't
really execercise the patchset without the fix (it's a bit sad that this
issue has gone unnoticed for months before I found it as part of the
development of this patch).

Think I'll push a minimal version now, and add an open item.


> 
> Regarding 0005:
> 
> There's sort of a mix of terminology here: are we pruning tuples or
> removing tuples or testing whether things are invisible? It would be
> better to be more consistent.
> 
> + * State for testing whether tuple versions may be removed. To improve
> + * GetSnapshotData() performance we don't compute an accurate value whenever
> + * acquiring a snapshot. Instead we compute boundaries above/below which we
> + * know that row versions are [not] needed anymore.  If at test time values
> + * falls in between the two, the boundaries can be recomputed (unless that
> + * just happened).
> 
> I don't like the wording here much. Maybe: State for testing whether
> an XID is invisible to all current snapshots. If an XID precedes
> maybe_needed_bound, it's definitely not visible to any current
> snapshot. If it equals or follows definitely_needed_bound, that XID
> isn't necessarily invisible to all snapshots. If it falls in between,
> we're not sure. If, when testing a particular tuple, we see an XID
> somewhere in the middle, we can try recomputing the boundaries to get
> a more accurate answer (unless we've just done that). This is cheaper
> than maintaining an accurate value all the time.

I'll incorporate that, thanks.


> There's also the problem that this sorta contradicts the comment for
> definitely_needed_bound. There it says intermediate values needed to
> be tested against the ProcArray, whereas here it says we need to
> recompute the bounds. That's kinda confusing.

For me those are the same. Computing an accurate bound is visitting the
procarray. But I'll rephrase.


> ComputedHorizons seems like a fairly generic name. I think there's
> some relationship between InvisibleToEveryoneState and
> ComputedHorizons that should be brought out more clearly by the naming
> and the comments.

I don't like the naming of ComputedHorizons, ComputeTransactionHorizons
much... But I find it hard to come up with something that's meaningfully
better.

I'll add a comment.


> + /*
> + * The value of ShmemVariableCache->latestCompletedFullXid when
> + * ComputeTransactionHorizons() held ProcArrayLock.
> + */
> + FullTransactionId latest_completed;
> +
> + /*
> + * The same for procArray->replication_slot_xmin and.
> + * procArray->replication_slot_catalog_xmin.
> + */
> + TransactionId slot_xmin;
> + TransactionId slot_catalog_xmin;
> 
> Department of randomly inconsistent names. In general I think it's
> quite hard to grasp the relationship between the different fields in
> ComputedHorizons.

What's the inconsistency? The dropped replication_ vs dropped FullXid
postfix?



> +
> +bool
> +GinPageIsRecyclable(Page page)
> 
> Needs a comment. Or more than one.

Well, I started to write one a couple times. But it's really just moving
the pre-existing code from the macro into a function and there weren't
any comments around *any* of it before. All my comment attempts
basically just were restating the code in so many words, or would have
required more work than I saw justified in the context of just moving
code.


> - /*
> - * If a transaction wrote a commit record in the gap between taking and
> - * logging the snapshot then latestCompletedXid may already be higher than
> - * the value from the snapshot, so check before we use the incoming value.
> - */
> - if (TransactionIdPrecedes(ShmemVariableCache->latestCompletedXid,
> -   running->latestCompletedXid))
> - ShmemVariableCache->latestCompletedXid = running->latestCompletedXid;
> -
> - Assert(TransactionIdIsNormal(ShmemVariableCache->latestCompletedXid));
> -
> - LWLockRelease(ProcArrayLock);
> 
> This code got relocated so that the lock is released later, but you
> didn't add any comments explaining why. Somebody will move it back and
> then you'll yet at them for doing it wrong. :-)

I just moved it because the code now references ->nextFullXid, which was
previously maintained after latestCompletedXid.


> + /*
> + * Must have called GetOldestVisibleTransactionId() if using SnapshotAny.
> + * Shouldn't have for an MVCC snapshot. (It's especially worth checking
> + * this for parallel builds, since ambuild routines that support parallel
> + * builds must work these details out for themselves.)
> + */
> + Assert(snapshot == SnapshotAny || IsMVCCSnapshot(snapshot));
> + Assert(snapshot == SnapshotAny ? TransactionIdIsValid(OldestXmin) :
> +    !TransactionIdIsValid(OldestXmin));
> + Assert(snapshot == SnapshotAny || !anyvisible);
> 
> This looks like a gratuitous code relocation.

I found it hard to understand the comments because the Asserts were done
further away from where the relevant decisions they were made. And I
think I have history to back me up: It looks to me that that that is
because ab0dfc961b6a821f23d9c40c723d11380ce195a6 just put the progress
related code between the if (!scan) and the Asserts.


> +HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer,
> TransactionId *dead_after)
> 
> I don't much like the name dead_after, but I don't have a better
> suggestion, either.
> 
> - * Deleter committed, but perhaps it was recent enough that some open
> - * transactions could still see the tuple.
> + * Deleter committed, allow caller to check if it was recent enough that
> + * some open transactions could still see the tuple.
> 
> I think you could drop this change.

Ok. Wasn't quite sure what to what to do with that comment.


> Generally, heap_prune_satisfies_vacuum looks pretty good. The
> limited_oldest_committed naming is confusing, but the comments make it
> a lot clearer.

I didn't like _committed much either. But couldn't come up with
something short. _relied_upon?


> + * If oldest btpo.xact in the deleted pages is invisible, then at
> 
> I'd say "invisible to everyone" here for clarity.
> 
> -latestCompletedXid variable.  This allows GetSnapshotData to use
> -latestCompletedXid + 1 as xmax for its snapshot: there can be no
> +latestCompletedFullXid variable.  This allows GetSnapshotData to use
> +latestCompletedFullXid + 1 as xmax for its snapshot: there can be no
> 
> Is this fixing a preexisting README defect?

It's just adjusting for the changed name of latestCompletedXid to
latestCompletedFullXid, as part of widening it to 64bits.  I'm not
really a fan of adding that to the variable name, but surrounding code
already did it (cf VariableCache->nextFullXid), so I thought I'd follow
suit.


> It might be useful if this README expanded on the new machinery a bit
> instead of just updating the wording to account for it, but I'm not
> sure exactly what that would look like or whether it would be too
> duplicative of other things.



> +void AssertTransactionIdMayBeOnDisk(TransactionId xid)
> 
> Formatting.
> 
> + * Assert that xid is one that we could actually see on disk.
> 
> I don't know what this means. The whole purpose of this routine is
> very unclear to me.

It's intended to be a double check against


>   * the secondary effect that it sets RecentGlobalXmin.  (This is critical
>   * for anything that reads heap pages, because HOT may decide to prune
>   * them even if the process doesn't attempt to modify any tuples.)
> + *
> + * FIXME: This comment is inaccurate / the code buggy. A snapshot that is
> + * not pushed/active does not reliably prevent HOT pruning (->xmin could
> + * e.g. be cleared when cache invalidations are processed).
> 
> Something needs to be done here... and in the other similar case.

Indeed. I wrote a separate email about it yesterday:
https://www.postgresql.org/message-id/20200407072418.ccvnyjbrktyi3rzc%40alap3.anarazel.de



> Is this kind of review helpful?

Yes!

Greetings,

Andres Freund