On Tue, May 24, 2022 at 11:28 AM Ranier Vilela <ranier...@gmail.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. 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. 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. 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. -- Robert Haas EDB: http://www.enterprisedb.com
