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
> Sounds like a plan! Do you want to write a patch?
Add the patch.
Attachments:
0001-use-pgxactoff-as-the-value-of-index-in-ProcArrayRemo.patchapplication/octet-stream; charset=utf-8; name=0001-use-pgxactoff-as-the-value-of-index-in-ProcArrayRemo.patchDownload
From f3176f831a110fc23c4d897f893e31400834a6a8 Mon Sep 17 00:00:00 2001
From: zhanyi <w@hidva.com>
Date: Fri, 7 May 2021 04:22:50 +0800
Subject: [PATCH] use pgxactoff as the value of index in ProcArrayRemove()
pgxactoff is always the index of pgprocno in procArray->pgprocnos.
---
src/backend/storage/ipc/procarray.c | 84 +++++++++++++++++------------
1 file changed, 49 insertions(+), 35 deletions(-)
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 5ff8cab394..d7cf4bdff0 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -511,6 +511,28 @@ ProcArrayAdd(PGPROC *proc)
LWLockRelease(ProcArrayLock);
}
+#ifdef USE_ASSERT_CHECKING
+static int
+find(int *array, int size, int val)
+{
+ for (int idx = 0; idx < size; ++idx)
+ {
+ if (array[idx] == val)
+ return idx;
+ }
+ return -1;
+}
+
+static int
+find_ex(int *array, int size, int start, int val)
+{
+ int ret = find(array + start, size - start, val);
+ if (ret >= 0)
+ ret += start;
+ return ret;
+}
+#endif
+
/*
* Remove the specified PGPROC from the shared array.
*
@@ -559,47 +581,39 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
Assert(!TransactionIdIsValid(ProcGlobal->xids[proc->pgxactoff]));
}
- Assert(TransactionIdIsValid(ProcGlobal->xids[proc->pgxactoff] == 0));
- Assert(TransactionIdIsValid(ProcGlobal->subxidStates[proc->pgxactoff].count == 0));
- Assert(TransactionIdIsValid(ProcGlobal->subxidStates[proc->pgxactoff].overflowed == false));
+ Assert(!TransactionIdIsValid(ProcGlobal->xids[proc->pgxactoff]));
+ Assert(ProcGlobal->subxidStates[proc->pgxactoff].count == 0);
+ Assert(ProcGlobal->subxidStates[proc->pgxactoff].overflowed == false);
ProcGlobal->statusFlags[proc->pgxactoff] = 0;
- for (index = 0; index < arrayP->numProcs; index++)
- {
- if (arrayP->pgprocnos[index] == proc->pgprocno)
- {
- /* Keep the PGPROC array sorted. See notes above */
- memmove(&arrayP->pgprocnos[index], &arrayP->pgprocnos[index + 1],
- (arrayP->numProcs - index - 1) * sizeof(*arrayP->pgprocnos));
- memmove(&ProcGlobal->xids[index], &ProcGlobal->xids[index + 1],
- (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->xids));
- memmove(&ProcGlobal->subxidStates[index], &ProcGlobal->subxidStates[index + 1],
- (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->subxidStates));
- memmove(&ProcGlobal->statusFlags[index], &ProcGlobal->statusFlags[index + 1],
- (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->statusFlags));
-
- arrayP->pgprocnos[arrayP->numProcs - 1] = -1; /* for debugging */
- arrayP->numProcs--;
-
- /* adjust for removed PGPROC */
- for (; index < arrayP->numProcs; index++)
- allProcs[arrayP->pgprocnos[index]].pgxactoff--;
+ index = proc->pgxactoff;
+ Assert(find(arrayP->pgprocnos, arrayP->numProcs, proc->pgprocno) == index);
+ Assert(find_ex(arrayP->pgprocnos, arrayP->numProcs, index + 1, proc->pgprocno) == -1);
- /*
- * Release in reversed acquisition order, to reduce frequency of
- * having to wait for XidGenLock while holding ProcArrayLock.
- */
- LWLockRelease(XidGenLock);
- LWLockRelease(ProcArrayLock);
- return;
- }
- }
+ /* Keep the PGPROC array sorted. See notes above */
+ memmove(&arrayP->pgprocnos[index], &arrayP->pgprocnos[index + 1],
+ (arrayP->numProcs - index - 1) * sizeof(*arrayP->pgprocnos));
+ memmove(&ProcGlobal->xids[index], &ProcGlobal->xids[index + 1],
+ (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->xids));
+ memmove(&ProcGlobal->subxidStates[index], &ProcGlobal->subxidStates[index + 1],
+ (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->subxidStates));
+ memmove(&ProcGlobal->statusFlags[index], &ProcGlobal->statusFlags[index + 1],
+ (arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->statusFlags));
- /* Oops */
+ arrayP->pgprocnos[arrayP->numProcs - 1] = -1; /* for debugging */
+ arrayP->numProcs--;
+
+ /* adjust for removed PGPROC */
+ for (; index < arrayP->numProcs; index++)
+ allProcs[arrayP->pgprocnos[index]].pgxactoff--;
+
+ /*
+ * Release in reversed acquisition order, to reduce frequency of
+ * having to wait for XidGenLock while holding ProcArrayLock.
+ */
LWLockRelease(XidGenLock);
LWLockRelease(ProcArrayLock);
-
- elog(LOG, "failed to find proc %p in ProcArray", proc);
+ return;
}
--
2.26.2
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
Ok! Thanks
----------
--------------原始邮件--------------
发件人："Andres Freund "<andres@anarazel.de>;
发送时间：2021年6月12日(星期六) 中午12:43
收件人："盏一" <w@hidva.com>;
抄送："pgsql-hackers "<pgsql-hackers@postgresql.org>;
主题：Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
-----------------------------------
Hi,
On 2021-05-07 04:36:25 +0800, 盏一 wrote:
> &gt;&nbsp;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
Import Notes
Resolved by subject fallback