Re: Improving connection scalability: GetSnapshotData()

Hi,

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.

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 ...

> Note you forward-declare struct GlobalVisState twice in heapam.h.

Oh, fixed, thanks.

I've also fixed a correctness bug that Thomas's cfbot found (and he
personally pointed out). There were occasional make check runs with
vacuum erroring out. That turned out to be because it was possible for
the horizon used to make decisions in heap_page_prune() and
lazy_scan_heap() to differ a bit. I've started a thread about my
concerns around the fragility of that logic [1]. The code around that
can use a bit more polish, I think. I mainly wanted to post a new
version so that the patch separated out above can be looked at.

Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/20200723181018.neey2jd3u7rfrfrn%40alap3.anarazel.de