Re: Improving connection scalability (src/backend/storage/ipc/procarray.c)

Em ter., 24 de mai. de 2022 às 13:06, Robert Haas <robertmhaas(at)gmail(dot)com>
escreveu:

> On Tue, May 24, 2022 at 11:28 AM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
> wrote:
> > I think that I got something.
>
> You might have something, but it's pretty hard to tell based on
> looking at this patch. Whatever relevant changes it has are mixed with
> a bunch of changes that are probably not relevant. For example, it's
> hard to believe that moving "uint32 i" to an inner scope in
> XidInMVCCSnapshot() is causing a performance gain, because an
> optimizing compiler should figure that out anyway.
>
I believe that even these small changes are helpful and favorable.
Improves code readability and helps the compiler generate better code,
especially for older compilers.

>
> An even bigger issue is that it's not sufficient to just demonstrate
> that the patch improves performance. It's also necessary to make an
> argument as to why it is safe and correct, and "I tried it out and
> nothing seemed to break" does not qualify as an argument.

Ok, certainly the convincing work is not good.

I'd guess that most or maybe all of the performance gain that you've
> observed
> here is attributable to changing GetSnapshotData() to call
> GetSnapshotDataReuse() without first acquiring ProcArrayLock.

It certainly helps, but I trust that's not the only reason, in all the
tests I did, there was an improvement in performance, even before using
this feature.
If you look closely at GetSnapShotData() you will see that
GetSnapshotDataReuse is called for all snapshots, even the new ones, which
is unnecessary.
Another example NormalTransactionIdPrecedes is more expensive than testing
statusFlags.

That
> doesn't seem like a completely hopeless idea, because the comments for
> GetSnapshotDataReuse() say this:
>
> * This very likely can be evolved to not need ProcArrayLock held (at very
> * least in the case we already hold a snapshot), but that's for another
> day.
>
> However, those comment seem to imply that it might not be safe in all
> cases, and that changes might be needed someplace in order to make it
> safe, but you haven't updated these comments, or changed the function
> in any way, so it's not really clear how or whether whatever problems
> Andres was worried about have been handled.
>
I think it's worth trying and testing to see if everything goes well,
so in the final patch apply whatever comments are needed.

regards,
Ranier Vilela