public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/3] Allow composite types in bootstrap 5+ messages / 3 participants [nested] [flat]
* [PATCH 2/3] Allow composite types in bootstrap @ 2020-11-17 15:28 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2020-11-17 15:28 UTC (permalink / raw) --- src/backend/bootstrap/bootstrap.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 18eb62ca47..e4fc75ab84 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -916,6 +916,7 @@ gettype(char *type) { if (Typ != NIL) { + static bool did_reread PG_USED_FOR_ASSERTS_ONLY = false; /* Already reread pg_types */ ListCell *lc; foreach (lc, Typ) @@ -927,6 +928,33 @@ gettype(char *type) return app->am_oid; } } + + /* + * The type wasn't known; check again to handle composite + * types, added since first populating the array. + */ + + /* + * Once all the types are populated and we handled composite + * types, shouldn't need to do that again. + */ + Assert(!did_reread); + did_reread = true; + + list_free_deep(Typ); + Typ = NULL; + populate_typ_array(); + + /* Need to avoid infinite recursion... */ + foreach (lc, Typ) + { + struct typmap *app = lfirst(lc); + if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0) + { + Ap = app; + return app->am_oid; + } + } } else { -- 2.26.2 --------------9399ACFA5093E21681352885 Content-Type: text/x-patch; charset=UTF-8; name="0003-Extended-statistics-on-expressions-20210117.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0003-Extended-statistics-on-expressions-20210117.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Refactor GetLockStatusData() by skipping unused backends and groups @ 2024-10-21 07:32 Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Bertrand Drouvot @ 2024-10-21 07:32 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers Hi, On Mon, Oct 21, 2024 at 09:19:49AM +0900, Fujii Masao wrote: > Hi, > > While reading the fast-path lock code, I noticed that GetLockStatusData() > checks all slots for every backend to gather fast-path lock data. However, > backends with PID=0 don't hold fast-path locks, right? I think the same as those are not a "regular" backend. > If so, we can > improve efficiency by having GetLockStatusData() skip those backends early. Agree. > Additionally, when GetLockStatusData() checks a backend, it currently > goes through all the slots accross its groups. Each group has 16 slots, > so if a backend has 4 groups (this can change depending on max_locks_per_transaction), > that means checking 64 slots. Instead, we could refactor the function > to skip groups without registered fast-path locks, improving performance. > Since each set of 16 slots is packed into a uint32 variable (PGPROC->fpLockBits[i]), > it’s easy to check if a group has any fast-path locks. > > I've attached a patch that implements these changes. This refactoring is > especially useful when max_connections and max_locks_per_transaction are > set high, as it reduces unnecessary checks across numerous slots. I think that your refactoring proposal makes sense. A few random comments on it: 1 === + /* Skip backends with pid=0, as they don't hold fast-path locks */ + if (proc->pid == 0) + continue; What about adding a few words in the comment that it represents prepared transactions? Or maybe add a new macro (say IS_PREPARED_TRANSACTION(proc)) and use it in the few places where we check for "PGPROC"->pid == 0 or "PGPROC"->pid != 0? 2 === - for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; ++f) + for (uint32 g = 0; g < FastPathLockGroupsPerBackend; g++) { As FP_LOCK_SLOTS_PER_BACKEND = (FP_LOCK_SLOTS_PER_GROUP * FastPathLockGroupsPerBackend) then the proposed approach starts with a "smaller" loop which makes sense. and then the patch: + for (int j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++) So that we are covering "FP_LOCK_SLOTS_PER_BACKEND" but discarded groups that do not contain registered fast-path locks: + /* Skip unallocated groups */ + if (proc->fpLockBits[g] == 0) That does make sense to me. One remark about the comment, what about? s/Skip unallocated groups/Skip groups without registered fast-path locks./? or at least add a "." at the end to be consistent with: "/* Skip unallocated slots. */" 3 === One thing that worry me a bit is that we "lost" the FP_LOCK_SLOTS_PER_BACKEND usage, so that if there is a change on it (for wathever reason) then we probably need to be careful that the change would be reflected here too. So, what about to add an Assert to check that we overall iterated over FP_LOCK_SLOTS_PER_BACKEND? 4 === Then the patch does move existing code around and just add a call to FAST_PATH_SLOT() to get fast-path lock slot index based on the group and slot indexes we are iterating on. That does make sense to me. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Refactor GetLockStatusData() by skipping unused backends and groups @ 2024-10-22 16:19 Fujii Masao <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Fujii Masao @ 2024-10-22 16:19 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: pgsql-hackers On 2024/10/21 16:32, Bertrand Drouvot wrote: > A few random comments on it: Thanks for the review! > 1 === > > + /* Skip backends with pid=0, as they don't hold fast-path locks */ > + if (proc->pid == 0) > + continue; > > What about adding a few words in the comment that it represents prepared > transactions? Or maybe add a new macro (say IS_PREPARED_TRANSACTION(proc)) and > use it in the few places where we check for "PGPROC"->pid == 0 or "PGPROC"->pid != 0? I understand that PGPROC entries with pid=0 are typically those not yet allocated to any backends. Yes, as you mentioned, prepared transactions also have pid=0. However, GetLockStatusData() loops up to ProcGlobal->allProcCount, which is MaxBackends plus NUM_AUXILIARY_PROCS, excluding prepared transactions. Therefore, GetLockStatusData() doesn't seem to check PGPROC entries for prepared transactions at all. In proc.c -------------- /* XXX allProcCount isn't really all of them; it excludes prepared xacts */ ProcGlobal->allProcCount = MaxBackends + NUM_AUXILIARY_PROCS; -------------- > One remark about the comment, what about? > > s/Skip unallocated groups/Skip groups without registered fast-path locks./? I've updated the source comment accordingly. > or at least add a "." at the end to be consistent with: > > "/* Skip unallocated slots. */" I removed the period at the end to match the usual convention in the codebase for single-line comment. I've attached v2 patch. > 3 === > > One thing that worry me a bit is that we "lost" the FP_LOCK_SLOTS_PER_BACKEND usage, > so that if there is a change on it (for wathever reason) then we probably need to > be careful that the change would be reflected here too. > > So, what about to add an Assert to check that we overall iterated over FP_LOCK_SLOTS_PER_BACKEND? You mean adding an assertion check to ensure that the slot ID calculated by FAST_PATH_SLOT() is less than FP_LOCK_SLOTS_PER_BACKEND? But GetLockStatusData() already calls FAST_PATH_GET_BITS() right after FAST_PATH_SLOT(), and FAST_PATH_GET_BITS() has an assertion that validates this. So, probably we can consider that this check is already in place. If it’s still worth adding, perhaps placing it inside the FAST_PATH_SLOT() macro could be an option... Or current assertion check is enough? Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION From b55620cf73d12ef95dc21f3b42afaa264500612d Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Sun, 20 Oct 2024 14:51:07 +0900 Subject: [PATCH v2] Refactor GetLockStatusData() to skip backends/groups without fast-path locks. Previously, GetLockStatusData() checked all slots for every backend to gather fast-path lock data, which could be inefficient. This commit refactors it by skipping backends with PID=0 (since they don't hold fast-path locks) and skipping groups with no registered fast-path locks, improving efficiency. This refactoring is particularly beneficial, for example when max_connections and max_locks_per_transaction are set high, as it reduces unnecessary checks across numerous slots. --- src/backend/storage/lmgr/lock.c | 67 +++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 09a8ac1578..4fccb7277e 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -3731,44 +3731,55 @@ GetLockStatusData(void) for (i = 0; i < ProcGlobal->allProcCount; ++i) { PGPROC *proc = &ProcGlobal->allProcs[i]; - uint32 f; + + /* Skip backends with pid=0, as they don't hold fast-path locks */ + if (proc->pid == 0) + continue; LWLockAcquire(&proc->fpInfoLock, LW_SHARED); - for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; ++f) + for (uint32 g = 0; g < FastPathLockGroupsPerBackend; g++) { - LockInstanceData *instance; - uint32 lockbits = FAST_PATH_GET_BITS(proc, f); - - /* Skip unallocated slots. */ - if (!lockbits) + /* Skip groups without registered fast-path locks */ + if (proc->fpLockBits[g] == 0) continue; - if (el >= els) + for (int j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++) { - els += MaxBackends; - data->locks = (LockInstanceData *) - repalloc(data->locks, sizeof(LockInstanceData) * els); - } + LockInstanceData *instance; + uint32 f = FAST_PATH_SLOT(g, j); + uint32 lockbits = FAST_PATH_GET_BITS(proc, f); - instance = &data->locks[el]; - SET_LOCKTAG_RELATION(instance->locktag, proc->databaseId, - proc->fpRelId[f]); - instance->holdMask = lockbits << FAST_PATH_LOCKNUMBER_OFFSET; - instance->waitLockMode = NoLock; - instance->vxid.procNumber = proc->vxid.procNumber; - instance->vxid.localTransactionId = proc->vxid.lxid; - instance->pid = proc->pid; - instance->leaderPid = proc->pid; - instance->fastpath = true; + /* Skip unallocated slots */ + if (!lockbits) + continue; - /* - * Successfully taking fast path lock means there were no - * conflicting locks. - */ - instance->waitStart = 0; + if (el >= els) + { + els += MaxBackends; + data->locks = (LockInstanceData *) + repalloc(data->locks, sizeof(LockInstanceData) * els); + } - el++; + instance = &data->locks[el]; + SET_LOCKTAG_RELATION(instance->locktag, proc->databaseId, + proc->fpRelId[f]); + instance->holdMask = lockbits << FAST_PATH_LOCKNUMBER_OFFSET; + instance->waitLockMode = NoLock; + instance->vxid.procNumber = proc->vxid.procNumber; + instance->vxid.localTransactionId = proc->vxid.lxid; + instance->pid = proc->pid; + instance->leaderPid = proc->pid; + instance->fastpath = true; + + /* + * Successfully taking fast path lock means there were no + * conflicting locks. + */ + instance->waitStart = 0; + + el++; + } } if (proc->fpVXIDLock) -- 2.46.2 Attachments: [text/plain] v2-0001-Refactor-GetLockStatusData-to-skip-backends-group.patch (3.5K, ../../[email protected]/2-v2-0001-Refactor-GetLockStatusData-to-skip-backends-group.patch) download | inline diff: From b55620cf73d12ef95dc21f3b42afaa264500612d Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Sun, 20 Oct 2024 14:51:07 +0900 Subject: [PATCH v2] Refactor GetLockStatusData() to skip backends/groups without fast-path locks. Previously, GetLockStatusData() checked all slots for every backend to gather fast-path lock data, which could be inefficient. This commit refactors it by skipping backends with PID=0 (since they don't hold fast-path locks) and skipping groups with no registered fast-path locks, improving efficiency. This refactoring is particularly beneficial, for example when max_connections and max_locks_per_transaction are set high, as it reduces unnecessary checks across numerous slots. --- src/backend/storage/lmgr/lock.c | 67 +++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 09a8ac1578..4fccb7277e 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -3731,44 +3731,55 @@ GetLockStatusData(void) for (i = 0; i < ProcGlobal->allProcCount; ++i) { PGPROC *proc = &ProcGlobal->allProcs[i]; - uint32 f; + + /* Skip backends with pid=0, as they don't hold fast-path locks */ + if (proc->pid == 0) + continue; LWLockAcquire(&proc->fpInfoLock, LW_SHARED); - for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; ++f) + for (uint32 g = 0; g < FastPathLockGroupsPerBackend; g++) { - LockInstanceData *instance; - uint32 lockbits = FAST_PATH_GET_BITS(proc, f); - - /* Skip unallocated slots. */ - if (!lockbits) + /* Skip groups without registered fast-path locks */ + if (proc->fpLockBits[g] == 0) continue; - if (el >= els) + for (int j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++) { - els += MaxBackends; - data->locks = (LockInstanceData *) - repalloc(data->locks, sizeof(LockInstanceData) * els); - } + LockInstanceData *instance; + uint32 f = FAST_PATH_SLOT(g, j); + uint32 lockbits = FAST_PATH_GET_BITS(proc, f); - instance = &data->locks[el]; - SET_LOCKTAG_RELATION(instance->locktag, proc->databaseId, - proc->fpRelId[f]); - instance->holdMask = lockbits << FAST_PATH_LOCKNUMBER_OFFSET; - instance->waitLockMode = NoLock; - instance->vxid.procNumber = proc->vxid.procNumber; - instance->vxid.localTransactionId = proc->vxid.lxid; - instance->pid = proc->pid; - instance->leaderPid = proc->pid; - instance->fastpath = true; + /* Skip unallocated slots */ + if (!lockbits) + continue; - /* - * Successfully taking fast path lock means there were no - * conflicting locks. - */ - instance->waitStart = 0; + if (el >= els) + { + els += MaxBackends; + data->locks = (LockInstanceData *) + repalloc(data->locks, sizeof(LockInstanceData) * els); + } - el++; + instance = &data->locks[el]; + SET_LOCKTAG_RELATION(instance->locktag, proc->databaseId, + proc->fpRelId[f]); + instance->holdMask = lockbits << FAST_PATH_LOCKNUMBER_OFFSET; + instance->waitLockMode = NoLock; + instance->vxid.procNumber = proc->vxid.procNumber; + instance->vxid.localTransactionId = proc->vxid.lxid; + instance->pid = proc->pid; + instance->leaderPid = proc->pid; + instance->fastpath = true; + + /* + * Successfully taking fast path lock means there were no + * conflicting locks. + */ + instance->waitStart = 0; + + el++; + } } if (proc->fpVXIDLock) -- 2.46.2 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Refactor GetLockStatusData() by skipping unused backends and groups @ 2024-10-24 02:12 Bertrand Drouvot <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Bertrand Drouvot @ 2024-10-24 02:12 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers Hi, On Wed, Oct 23, 2024 at 01:19:37AM +0900, Fujii Masao wrote: > > I understand that PGPROC entries with pid=0 are typically those not yet allocated to > any backends. Yes, as you mentioned, prepared transactions also have pid=0. However, > GetLockStatusData() loops up to ProcGlobal->allProcCount, which is MaxBackends plus > NUM_AUXILIARY_PROCS, excluding prepared transactions. Therefore, GetLockStatusData() > doesn't seem to check PGPROC entries for prepared transactions at all. > > In proc.c > -------------- > /* XXX allProcCount isn't really all of them; it excludes prepared xacts */ > ProcGlobal->allProcCount = MaxBackends + NUM_AUXILIARY_PROCS; > -------------- Oh right, thanks for pointing out! > I removed the period at the end to match the usual convention in the codebase > for single-line comment. > > I've attached v2 patch. Thanks for the new version! > You mean adding an assertion check to ensure that the slot ID calculated by > FAST_PATH_SLOT() is less than FP_LOCK_SLOTS_PER_BACKEND? Yes. > But GetLockStatusData() > already calls FAST_PATH_GET_BITS() right after FAST_PATH_SLOT(), > and FAST_PATH_GET_BITS() has an assertion that validates this. Oh right, it's "already" in FAST_PATH_GROUP() (and FAST_PATH_INDEX()). > So, probably > we can consider that this check is already in place. If it’s still worth adding, > perhaps placing it inside the FAST_PATH_SLOT() macro could be an option... > Or current assertion check is enough? Thought? Given that it's already done in FAST_PATH_GET_BITS(), I think that's fine as it is and v2 LGTM. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Refactor GetLockStatusData() by skipping unused backends and groups @ 2024-10-24 15:23 Fujii Masao <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Fujii Masao @ 2024-10-24 15:23 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: pgsql-hackers On 2024/10/24 11:12, Bertrand Drouvot wrote: >> So, probably >> we can consider that this check is already in place. If it’s still worth adding, >> perhaps placing it inside the FAST_PATH_SLOT() macro could be an option... >> Or current assertion check is enough? Thought? > > Given that it's already done in FAST_PATH_GET_BITS(), I think that's fine as it > is and v2 LGTM. Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-10-24 15:23 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-17 15:28 [PATCH 2/3] Allow composite types in bootstrap Justin Pryzby <[email protected]> 2024-10-21 07:32 Re: Refactor GetLockStatusData() by skipping unused backends and groups Bertrand Drouvot <[email protected]> 2024-10-22 16:19 ` Re: Refactor GetLockStatusData() by skipping unused backends and groups Fujii Masao <[email protected]> 2024-10-24 02:12 ` Re: Refactor GetLockStatusData() by skipping unused backends and groups Bertrand Drouvot <[email protected]> 2024-10-24 15:23 ` Re: Refactor GetLockStatusData() by skipping unused backends and groups Fujii Masao <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox