On Thu, Sep 07, 2023 at 01:22:07PM +0530, Ashutosh Bapat wrote: > Otherwise the patch looks good to me. > > The function modified by the patch is only used by extension > pgrowlocks. Given that the function will be invoked as many times as > the number of locked rows in the relation, the patch may show some > improvement and thus be more compelling. One way to measure > performance is to create a table with millions of rows, SELECT all > rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that > relation. This will invoke the given function a million times. That > way we might be able to catch some miniscule improvement per row. > > If the performance is measurable, we can mark the CF entry as ready > for committer.
So, is the difference measurable? Assuming that your compiler does not optimize that, my guess is no because the cycles are going to be eaten by the other system calls in pgrowlocks. You could increase the number of proc entries and force a large loop around BackendXidGetPid() to see a difference of runtime, but that's going to require a lot more proc entries to see any kind of difference outside the scope of a usual ~1% (somewhat magic number!) noise with such micro benchmarks. - int pgprocno = arrayP->pgprocnos[index]; - PGPROC *proc = &allProcs[pgprocno]; - if (other_xids[index] == xid) { + PGPROC *proc = &allProcs[arrayP->pgprocnos[index]]; result = proc->pid; break; Saying that, moving the declarations into the inner loop is usually a good practice, but I would just keep two variables instead of one for the sake of readability. That's a nit, sure. -- Michael
signature.asc
Description: PGP signature