Re: Improving connection scalability: GetSnapshotData()

Hi,

On 2020-08-16 14:11:46 -0700, Andres Freund wrote:
> On 2020-08-16 13:52:58 -0700, Andres Freund wrote:
> > On 2020-08-16 13:31:53 -0700, Andres Freund wrote:
> > Gna, I think I see the problem. In at least one place I wrongly
> > accessed the 'dense' array of in-progress xids using the 'pgprocno',
> > instead of directly using the [0...procArray->numProcs) index.
> >
> > Working on a fix, together with some improved asserts.
>
> diff --git i/src/backend/storage/ipc/procarray.c w/src/backend/storage/ipc/procarray.c
> index 8262abd42e6..96e4a878576 100644
> --- i/src/backend/storage/ipc/procarray.c
> +++ w/src/backend/storage/ipc/procarray.c
> @@ -1663,7 +1663,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
> TransactionId xmin;
>
> /* Fetch xid just once - see GetNewTransactionId */
> - xid = UINT32_ACCESS_ONCE(other_xids[pgprocno]);
> + xid = UINT32_ACCESS_ONCE(other_xids[index]);
> xmin = UINT32_ACCESS_ONCE(proc->xmin);
>
> /*
>
> indeed fixes the issue based on a number of iterations of your modified
> test, and fixes a clear bug.

Pushed that much.

> WRT better asserts: We could make ProcArrayRemove() and InitProcGlobal()
> initialize currently unused procArray->pgprocnos,
> procGlobal->{xids,subxidStates,vacuumFlags} to invalid values and/or
> declare them as uninitialized using the valgrind helpers.
>
> For the first, one issue is that there's no obviously good candidate for
> an uninitialized xid. We could use something like FrozenTransactionId,
> which may never be in the procarray. But it's not exactly pretty.
>
> Opinions?

So we get some builfarm results while thinking about this.

Greetings,

Andres Freund