Thread: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`

Hi,

Since we have introduced `pgxactoff` in
[941697c3c1ae5d6ee153065adb96e1e63ee11224](https://github.com/postgres/postgres/commit/941697c3c1ae5d6ee153065adb96e1e63ee11224),
and`pgxactoff` is always the index of `proc->pgprocno` in `procArray->pgprocnos`. So it seems that we could directly
use`proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`? My thought is to replace 
 

```c
for (index = 0; index < arrayP->numProcs; index++)
{
  if (arrayP->pgprocnos[index] == proc->pgprocno)
  {
      /* ... */
  }
}
```

with 

```c
index = proc->pgxactoff;
/* ... */
```

I would appreciate your help.

Hi,

On 2021-05-07 00:30:13 +0800, 盏一 wrote:
> Since we have introduced `pgxactoff` in
[941697c3c1ae5d6ee153065adb96e1e63ee11224](https://github.com/postgres/postgres/commit/941697c3c1ae5d6ee153065adb96e1e63ee11224),
and`pgxactoff` is always the index of `proc->pgprocno` in `procArray->pgprocnos`. So it seems that we could directly
use`proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`? My thought is to replace
 
>
> ```c
> for (index = 0; index < arrayP->numProcs; index++)
> {
>   if (arrayP->pgprocnos[index] == proc->pgprocno)
>   {
>       /* ... */
>   }
> }
> ```
>
> with
>
> ```c
> index = proc->pgxactoff;
> /* ... */
> ```

Sounds like a plan! Do you want to write a patch?

If you do, I think it might be worthwhile to add an only-with-assertions
loop checking that there's no other entry with the same pgprocno in the
dense arrays.

Given that the code is new in 14, I wonder if we should cram this
simplification in before beta? I don't think this is likely to matter
performance wise, but it seems like it'll make maintenance easier to not
have it look different in 14 than it does both in 13 and 15.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2021-05-07 00:30:13 +0800, 盏一 wrote:
>> Since we have introduced `pgxactoff` in
[941697c3c1ae5d6ee153065adb96e1e63ee11224](https://github.com/postgres/postgres/commit/941697c3c1ae5d6ee153065adb96e1e63ee11224),
and`pgxactoff` is always the index of `proc->pgprocno` in `procArray->pgprocnos`. So it seems that we could directly
use`proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`? My thought is to replace 

> Sounds like a plan! Do you want to write a patch?

> If you do, I think it might be worthwhile to add an only-with-assertions
> loop checking that there's no other entry with the same pgprocno in the
> dense arrays.

Hmm, I can definitely see keeping a check that the selected entry
has the right PID and/or pgprocno, but making it search for duplicates
seems a bit over the top.  The existing code isn't guarding against
that, and I don't really see a reason why there's a meaningful risk
of it.

> Given that the code is new in 14, I wonder if we should cram this
> simplification in before beta?

+1, seems like a pretty clear missed opportunity in 941697c3c.

            regards, tom lane

Hi,

On 2021-05-06 15:27:29 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > If you do, I think it might be worthwhile to add an only-with-assertions
> > loop checking that there's no other entry with the same pgprocno in the
> > dense arrays.
>
> Hmm, I can definitely see keeping a check that the selected entry
> has the right PID and/or pgprocno, but making it search for duplicates
> seems a bit over the top.  The existing code isn't guarding against
> that, and I don't really see a reason why there's a meaningful risk
> of it.

The current code makes it at least more likely for things to fall over
badly if there's such an issue, because there's a 50/50 chance that the
wrong entry would be moved.  I do dimly remember hitting a nasty bug or
two during the development of 941697c3c where such a thing happened, but
I don't remember the details.

Greetings,

Andres Freund

On 5/6/21 3:27 PM, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2021-05-07 00:30:13 +0800, 盏一 wrote:
>>> Since we have introduced `pgxactoff` in
[941697c3c1ae5d6ee153065adb96e1e63ee11224](https://github.com/postgres/postgres/commit/941697c3c1ae5d6ee153065adb96e1e63ee11224),
and`pgxactoff` is always the index of `proc->pgprocno` in `procArray->pgprocnos`. So it seems that we could directly
use`proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`? My thought is to replace
 
>> Sounds like a plan! Do you want to write a patch?
>> If you do, I think it might be worthwhile to add an only-with-assertions
>> loop checking that there's no other entry with the same pgprocno in the
>> dense arrays.
> Hmm, I can definitely see keeping a check that the selected entry
> has the right PID and/or pgprocno, but making it search for duplicates
> seems a bit over the top.  The existing code isn't guarding against
> that, and I don't really see a reason why there's a meaningful risk
> of it.
>
>> Given that the code is new in 14, I wonder if we should cram this
>> simplification in before beta?
> +1, seems like a pretty clear missed opportunity in 941697c3c.
>
>             


open item then?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2021-05-07 04:36:25 +0800, 盏一 wrote:
> > Sounds like a plan! Do you want to write a patch?
> Add the patch.

Thanks for the patch. I finally pushed an edited version of it. There
were other loops over ->pgprocnos, so I put assertions in those - that
gains us a a good bit more checking than we had before...

I also couldn't resist to do some small formatting cleanups - I found
the memmove calls just too hard to read.

I took the authorship information as you had it in the diff you attached
- I hope that's OK?

Greetings,

Andres Freund