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
