pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feeduse `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
8+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2021-06-12 d8e950d3ae Improve and cleanup ProcArrayAdd(), ProcArrayRemove().
* use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-05-06 16:30 盏一 <w@hidva.com>
0 siblings, 1 reply; 8+ messages in thread
From: 盏一 @ 2021-05-06 16:30 UTC (permalink / raw)
To: pgsql-hackers; +Cc: andres <andres@anarazel.de>
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.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-05-06 19:08 Andres Freund <andres@anarazel.de>
parent: 盏一 <w@hidva.com>
0 siblings, 2 replies; 8+ messages in thread
From: Andres Freund @ 2021-05-06 19:08 UTC (permalink / raw)
To: 盏一 <w@hidva.com>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-05-06 19:27 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 2 replies; 8+ messages in thread
From: Tom Lane @ 2021-05-06 19:27 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: 盏一 <w@hidva.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-05-06 19:45 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 8+ messages in thread
From: Andres Freund @ 2021-05-06 19:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: 盏一 <w@hidva.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-05-06 20:16 Andrew Dunstan <andrew@dunslane.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Dunstan @ 2021-05-06 20:16 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; +Cc: 盏一 <w@hidva.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-05-06 20:36 盏一 <w@hidva.com>
parent: Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 8+ messages in thread
From: 盏一 @ 2021-05-06 20:36 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; pgsql-hackers
> 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.patch (application/octet-stream, 4.3K) download
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
* Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-06-12 04:42 Andres Freund <andres@anarazel.de>
parent: 盏一 <w@hidva.com>
0 siblings, 0 replies; 8+ messages in thread
From: Andres Freund @ 2021-06-12 04:42 UTC (permalink / raw)
To: 盏一 <w@hidva.com>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()`
@ 2021-06-12 04:52 盏一 <w@hidva.com>
0 siblings, 0 replies; 8+ messages in thread
From: 盏一 @ 2021-06-12 04:52 UTC (permalink / raw)
To: andres freund <andres@anarazel.de>; +Cc: pgsql-hackers
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:
> > 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
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2021-06-12 04:52 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 16:30 use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()` 盏一 <w@hidva.com>
2021-05-06 19:08 ` Andres Freund <andres@anarazel.de>
2021-05-06 19:27 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-06 19:45 ` Andres Freund <andres@anarazel.de>
2021-05-06 20:16 ` Andrew Dunstan <andrew@dunslane.net>
2021-05-06 20:36 ` 盏一 <w@hidva.com>
2021-06-12 04:42 ` Andres Freund <andres@anarazel.de>
2021-06-12 04:52 Re: use `proc->pgxactoff` as the value of `index` in `ProcArrayRemove()` 盏一 <w@hidva.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox