Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
Ok! Thanks -- --原始邮件-- 发件人："Andres Freund "
Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
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
Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
Sounds like a plan! Do you want to write a patch? Add the patch. 0001-use-pgxactoff-as-the-value-of-index-in-ProcArrayRemo.patch Description: Binary data
Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
On 5/6/21 3:27 PM, Tom Lane wrote: > Andres Freund 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
Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
Hi, On 2021-05-06 15:27:29 -0400, Tom Lane wrote: > Andres Freund 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
Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
Andres Freund 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
Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
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
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.
