public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 1/2] mark_async_capable(): subpath should match subplan
20+ messages / 4 participants
[nested] [flat]
* [PATCH v4 1/2] mark_async_capable(): subpath should match subplan
@ 2023-07-21 09:05 Alexander Pyhalov <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Pyhalov @ 2023-07-21 09:05 UTC (permalink / raw)
---
src/backend/optimizer/plan/createplan.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 63fe6637155..0053befc8db 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1139,10 +1139,10 @@ mark_async_capable_plan(Plan *plan, Path *path)
SubqueryScan *scan_plan = (SubqueryScan *) plan;
/*
- * If the generated plan node includes a gating Result node,
- * we can't execute it asynchronously.
+ * If the generated plan node includes a gating Result node or
+ * a Sort node, we can't execute it asynchronously.
*/
- if (IsA(plan, Result))
+ if (IsA(plan, Result) || IsA(plan, Sort))
return false;
/*
@@ -1160,10 +1160,10 @@ mark_async_capable_plan(Plan *plan, Path *path)
FdwRoutine *fdwroutine = path->parent->fdwroutine;
/*
- * If the generated plan node includes a gating Result node,
- * we can't execute it asynchronously.
+ * If the generated plan node includes a gating Result node or
+ * a Sort node, we can't execute it asynchronously.
*/
- if (IsA(plan, Result))
+ if (IsA(plan, Result) || IsA(plan, Sort))
return false;
Assert(fdwroutine != NULL);
@@ -1176,9 +1176,9 @@ mark_async_capable_plan(Plan *plan, Path *path)
/*
* If the generated plan node includes a Result node for the
- * projection, we can't execute it asynchronously.
+ * projection or a Sort node, we can't execute it asynchronously.
*/
- if (IsA(plan, Result))
+ if (IsA(plan, Result) || IsA(plan, Sort))
return false;
/*
--
2.47.3
--nfti2epu7uxujyyg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v4-0002-MergeAppend-should-support-Async-Foreign-Scan-sub.patch"
^ permalink raw reply [nested|flat] 20+ messages in thread
* [PATCH v4 1/2] mark_async_capable(): subpath should match subplan
@ 2023-07-21 09:05 Alexander Pyhalov <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Pyhalov @ 2023-07-21 09:05 UTC (permalink / raw)
---
src/backend/optimizer/plan/createplan.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 63fe6637155..0053befc8db 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1139,10 +1139,10 @@ mark_async_capable_plan(Plan *plan, Path *path)
SubqueryScan *scan_plan = (SubqueryScan *) plan;
/*
- * If the generated plan node includes a gating Result node,
- * we can't execute it asynchronously.
+ * If the generated plan node includes a gating Result node or
+ * a Sort node, we can't execute it asynchronously.
*/
- if (IsA(plan, Result))
+ if (IsA(plan, Result) || IsA(plan, Sort))
return false;
/*
@@ -1160,10 +1160,10 @@ mark_async_capable_plan(Plan *plan, Path *path)
FdwRoutine *fdwroutine = path->parent->fdwroutine;
/*
- * If the generated plan node includes a gating Result node,
- * we can't execute it asynchronously.
+ * If the generated plan node includes a gating Result node or
+ * a Sort node, we can't execute it asynchronously.
*/
- if (IsA(plan, Result))
+ if (IsA(plan, Result) || IsA(plan, Sort))
return false;
Assert(fdwroutine != NULL);
@@ -1176,9 +1176,9 @@ mark_async_capable_plan(Plan *plan, Path *path)
/*
* If the generated plan node includes a Result node for the
- * projection, we can't execute it asynchronously.
+ * projection or a Sort node, we can't execute it asynchronously.
*/
- if (IsA(plan, Result))
+ if (IsA(plan, Result) || IsA(plan, Sort))
return false;
/*
--
2.47.3
--nfti2epu7uxujyyg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v4-0002-MergeAppend-should-support-Async-Foreign-Scan-sub.patch"
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-21 20:40 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-21 20:40 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Thu, Mar 21, 2024 at 04:31:45PM -0400, Tom Lane wrote:
> I wrote:
>> ... I still see the problematic GRANT taking ~250ms, compared
>> to 5ms in v15. roles_is_member_of is clearly on the hook for that.
>
> Ah: looks like that is mainly the fault of the list_append_unique_oid
> calls in roles_is_member_of. That's also an O(N^2) cost of course,
> though with a much smaller constant factor.
>
> I don't think we have any really cheap way to de-duplicate the role
> OIDs, especially seeing that it has to be done on-the-fly within the
> collection loop, and the order of roles_list is at least potentially
> interesting. Not sure how to make further progress without a lot of
> work.
Assuming these are larger lists, this might benefit from optimizations
involving SIMD intrinsics. I looked into that a while ago [0], but the
effort was abandoned because we didn't have any concrete use-cases at the
time. (I'm looking into some additional optimizations in a separate thread
[1] that would likely apply here, too.)
[0] https://postgr.es/m/20230308002502.GA3378449%40nathanxps13
[1] https://postgr.es/m/20240321183823.GA1800896%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 00:51 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 00:51 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Thu, Mar 21, 2024 at 03:40:12PM -0500, Nathan Bossart wrote:
> On Thu, Mar 21, 2024 at 04:31:45PM -0400, Tom Lane wrote:
>> I don't think we have any really cheap way to de-duplicate the role
>> OIDs, especially seeing that it has to be done on-the-fly within the
>> collection loop, and the order of roles_list is at least potentially
>> interesting. Not sure how to make further progress without a lot of
>> work.
>
> Assuming these are larger lists, this might benefit from optimizations
> involving SIMD intrinsics. I looked into that a while ago [0], but the
> effort was abandoned because we didn't have any concrete use-cases at the
> time. (I'm looking into some additional optimizations in a separate thread
> [1] that would likely apply here, too.)
Never mind. With the reproduction script, I'm only seeing a ~2%
improvement with my patches.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 00:59 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 3 replies; 20+ messages in thread
From: Tom Lane @ 2024-03-22 00:59 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
Nathan Bossart <[email protected]> writes:
> On Thu, Mar 21, 2024 at 03:40:12PM -0500, Nathan Bossart wrote:
>> On Thu, Mar 21, 2024 at 04:31:45PM -0400, Tom Lane wrote:
>>> I don't think we have any really cheap way to de-duplicate the role
>>> OIDs, especially seeing that it has to be done on-the-fly within the
>>> collection loop, and the order of roles_list is at least potentially
>>> interesting. Not sure how to make further progress without a lot of
>>> work.
>> Assuming these are larger lists, this might benefit from optimizations
>> involving SIMD intrinsics.
> Never mind. With the reproduction script, I'm only seeing a ~2%
> improvement with my patches.
Yeah, you cannot beat an O(N^2) problem by throwing SIMD at it.
However ... I just remembered that we have a Bloom filter implementation
in core now (src/backend/lib/bloomfilter.c). How about using that
to quickly reject (hopefully) most role OIDs, and only do the
list_member_oid check if the filter passes?
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 01:03 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 01:03 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Thu, Mar 21, 2024 at 08:59:54PM -0400, Tom Lane wrote:
> However ... I just remembered that we have a Bloom filter implementation
> in core now (src/backend/lib/bloomfilter.c). How about using that
> to quickly reject (hopefully) most role OIDs, and only do the
> list_member_oid check if the filter passes?
Seems worth a try. I've been looking for an excuse to use that...
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 01:52 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 01:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Thu, Mar 21, 2024 at 08:03:32PM -0500, Nathan Bossart wrote:
> On Thu, Mar 21, 2024 at 08:59:54PM -0400, Tom Lane wrote:
>> However ... I just remembered that we have a Bloom filter implementation
>> in core now (src/backend/lib/bloomfilter.c). How about using that
>> to quickly reject (hopefully) most role OIDs, and only do the
>> list_member_oid check if the filter passes?
>
> Seems worth a try. I've been looking for an excuse to use that...
The Bloom filter appears to help a bit, although it regresses the
create-roles.sql portion of the test. I'm assuming that's thanks to all
the extra pallocs and pfrees, which are probably avoidable if we store the
filter in a long-lived context and just clear it at the beginning of each
call to roles_is_member_of().
HEAD hash hash+bloom
create 2.02 2.06 2.92
grant 4.63 0.27 0.08
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] bloom.patch (2.1K, ../../20240322015251.GA2193900@nathanxps13/2-bloom.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index cf5d08576a..445a14646c 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -36,6 +36,7 @@
#include "common/hashfn.h"
#include "foreign/foreign.h"
#include "funcapi.h"
+#include "lib/bloomfilter.h"
#include "lib/qunique.h"
#include "miscadmin.h"
#include "utils/acl.h"
@@ -4946,6 +4947,7 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
ListCell *l;
List *new_cached_roles;
MemoryContext oldctx;
+ bloom_filter *bf;
Assert(OidIsValid(admin_of) == PointerIsValid(admin_role));
if (admin_role != NULL)
@@ -4987,6 +4989,9 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
*/
roles_list = list_make1_oid(roleid);
+ bf = bloom_create(1024, work_mem, 0);
+ bloom_add_element(bf, (unsigned char *) &roleid, sizeof(Oid));
+
foreach(l, roles_list)
{
Oid memberid = lfirst_oid(l);
@@ -5023,16 +5028,29 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
* graph, we must test for having already seen this role. It is
* legal for instance to have both A->B and A->C->B.
*/
- roles_list = list_append_unique_oid(roles_list, otherid);
+ if (bloom_lacks_element(bf, (unsigned char *) &otherid, sizeof(Oid)))
+ roles_list = lappend_oid(roles_list, otherid);
+ else
+ roles_list = list_append_unique_oid(roles_list, otherid);
+ bloom_add_element(bf, (unsigned char *) &otherid, sizeof(Oid));
}
ReleaseSysCacheList(memlist);
/* implement pg_database_owner implicit membership */
if (memberid == dba && OidIsValid(dba))
- roles_list = list_append_unique_oid(roles_list,
- ROLE_PG_DATABASE_OWNER);
+ {
+ Oid dbowner = ROLE_PG_DATABASE_OWNER;
+
+ if (bloom_lacks_element(bf, (unsigned char *) &dbowner, sizeof(Oid)))
+ roles_list = lappend_oid(roles_list, ROLE_PG_DATABASE_OWNER);
+ else
+ roles_list = list_append_unique_oid(roles_list, ROLE_PG_DATABASE_OWNER);
+ bloom_add_element(bf, (unsigned char *) &dbowner, sizeof(Oid));
+ }
}
+ bloom_free(bf);
+
/*
* Copy the completed list into TopMemoryContext so it will persist.
*/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 02:02 alex work <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 0 replies; 20+ messages in thread
From: alex work @ 2024-03-22 02:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; [email protected]; Robert Haas <[email protected]>
First of all thank you for looking into this.
At the moment we workaround the problem by altering `acc` ROLE into a SUPERUSER
in PostgreSQL 16 instances. It sidestep the problem and having the lowest cost
to implement for us. While at first we think this feels like opening a security
hole, it does not introduce side effects for **our use case** by the way our
application make use of this `acc` ROLE.
Of course we cannot recommend the workaround we took to others having similar
situation.
On Fri, Mar 22, 2024 at 7:59 AM Tom Lane <[email protected]> wrote:
>
> Nathan Bossart <[email protected]> writes:
> > On Thu, Mar 21, 2024 at 03:40:12PM -0500, Nathan Bossart wrote:
> >> On Thu, Mar 21, 2024 at 04:31:45PM -0400, Tom Lane wrote:
> >>> I don't think we have any really cheap way to de-duplicate the role
> >>> OIDs, especially seeing that it has to be done on-the-fly within the
> >>> collection loop, and the order of roles_list is at least potentially
> >>> interesting. Not sure how to make further progress without a lot of
> >>> work.
>
> >> Assuming these are larger lists, this might benefit from optimizations
> >> involving SIMD intrinsics.
>
> > Never mind. With the reproduction script, I'm only seeing a ~2%
> > improvement with my patches.
>
> Yeah, you cannot beat an O(N^2) problem by throwing SIMD at it.
>
> However ... I just remembered that we have a Bloom filter implementation
> in core now (src/backend/lib/bloomfilter.c). How about using that
> to quickly reject (hopefully) most role OIDs, and only do the
> list_member_oid check if the filter passes?
>
> regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 02:07 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Tom Lane @ 2024-03-22 02:07 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
Nathan Bossart <[email protected]> writes:
> The Bloom filter appears to help a bit, although it regresses the
> create-roles.sql portion of the test. I'm assuming that's thanks to all
> the extra pallocs and pfrees, which are probably avoidable if we store the
> filter in a long-lived context and just clear it at the beginning of each
> call to roles_is_member_of().
The zero-fill to reinitialize the filter probably costs a good deal
all by itself, considering we're talking about at least a megabyte.
Maybe a better idea is to not enable the filter till we're dealing
with at least 1000 or so entries in roles_list, though obviously that
will complicate the logic.
+ if (bloom_lacks_element(bf, (unsigned char *) &otherid, sizeof(Oid)))
+ roles_list = lappend_oid(roles_list, otherid);
+ else
+ roles_list = list_append_unique_oid(roles_list, otherid);
+ bloom_add_element(bf, (unsigned char *) &otherid, sizeof(Oid));
Hmm, I was imagining something more like
if (bloom_lacks_element(bf, (unsigned char *) &otherid, sizeof(Oid)) ||
!list_member_oid(roles_list, otherid))
{
roles_list = lappend_oid(roles_list, otherid);
bloom_add_element(bf, (unsigned char *) &otherid, sizeof(Oid));
}
to avoid duplicate bloom_add_element calls. Probably wouldn't move
the needle much in this specific test case, but this formulation
would simplify letting the filter kick in later. Very roughly,
if ((bf && bloom_lacks_element(bf, (unsigned char *) &otherid, sizeof(Oid))) ||
!list_member_oid(roles_list, otherid))
{
if (bf == NULL && list_length(roles_list) > 1000)
{
... create bf and populate with existing list entries
}
roles_list = lappend_oid(roles_list, otherid);
if (bf)
bloom_add_element(bf, (unsigned char *) &otherid, sizeof(Oid));
}
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 14:47 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 14:47 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Thu, Mar 21, 2024 at 08:59:54PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> On Thu, Mar 21, 2024 at 03:40:12PM -0500, Nathan Bossart wrote:
>>> On Thu, Mar 21, 2024 at 04:31:45PM -0400, Tom Lane wrote:
>>>> I don't think we have any really cheap way to de-duplicate the role
>>>> OIDs, especially seeing that it has to be done on-the-fly within the
>>>> collection loop, and the order of roles_list is at least potentially
>>>> interesting. Not sure how to make further progress without a lot of
>>>> work.
>
>>> Assuming these are larger lists, this might benefit from optimizations
>>> involving SIMD intrinsics.
>
>> Never mind. With the reproduction script, I'm only seeing a ~2%
>> improvement with my patches.
>
> Yeah, you cannot beat an O(N^2) problem by throwing SIMD at it.
I apparently had some sort of major brain fade when I did this because I
didn't apply your hashing patch when I ran this SIMD test. With it
applied, I see a speedup of ~39%, which makes a whole lot more sense to me.
If I add the Bloom patch (updated with your suggestions), I get another
~73% improvement from there, and a much smaller regression in the role
creation portion.
hash hash+simd hash+simd+bloom
create 1.27 1.27 1.28
grant 0.18 0.11 0.03
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] bloom_v2.patch (2.3K, ../../20240322144739.GA2321246@nathanxps13/2-bloom_v2.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index cf5d08576a..4952dc9c1e 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -36,6 +36,7 @@
#include "common/hashfn.h"
#include "foreign/foreign.h"
#include "funcapi.h"
+#include "lib/bloomfilter.h"
#include "lib/qunique.h"
#include "miscadmin.h"
#include "utils/acl.h"
@@ -4946,6 +4947,7 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
ListCell *l;
List *new_cached_roles;
MemoryContext oldctx;
+ bloom_filter *bf = NULL;
Assert(OidIsValid(admin_of) == PointerIsValid(admin_role));
if (admin_role != NULL)
@@ -5023,16 +5025,46 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
* graph, we must test for having already seen this role. It is
* legal for instance to have both A->B and A->C->B.
*/
- roles_list = list_append_unique_oid(roles_list, otherid);
+ if ((bf && bloom_lacks_element(bf, (unsigned char *) &otherid, sizeof(Oid))) ||
+ !list_member_oid(roles_list, otherid))
+ {
+ if (bf == NULL && list_length(roles_list) > 1000)
+ {
+ bf = bloom_create(1024 * 1024, work_mem, 0);
+ foreach_oid(role, roles_list)
+ bloom_add_element(bf, (unsigned char *) &role, sizeof(Oid));
+ }
+ roles_list = lappend_oid(roles_list, otherid);
+ if (bf)
+ bloom_add_element(bf, (unsigned char *) &otherid, sizeof(Oid));
+ }
}
ReleaseSysCacheList(memlist);
/* implement pg_database_owner implicit membership */
if (memberid == dba && OidIsValid(dba))
- roles_list = list_append_unique_oid(roles_list,
- ROLE_PG_DATABASE_OWNER);
+ {
+ Oid dbowner = ROLE_PG_DATABASE_OWNER;
+
+ if ((bf && bloom_lacks_element(bf, (unsigned char *) &dbowner, sizeof(Oid))) ||
+ !list_member_oid(roles_list, dbowner))
+ {
+ if (bf == NULL && list_length(roles_list) > 1000)
+ {
+ bf = bloom_create(1024 * 1024, work_mem, 0);
+ foreach_oid(role, roles_list)
+ bloom_add_element(bf, (unsigned char *) &role, sizeof(Oid));
+ }
+ roles_list = lappend_oid(roles_list, dbowner);
+ if (bf)
+ bloom_add_element(bf, (unsigned char *) &dbowner, sizeof(Oid));
+ }
+ }
}
+ if (bf)
+ bloom_free(bf);
+
/*
* Copy the completed list into TopMemoryContext so it will persist.
*/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 14:55 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 14:55 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Fri, Mar 22, 2024 at 09:47:39AM -0500, Nathan Bossart wrote:
> hash hash+simd hash+simd+bloom
> create 1.27 1.27 1.28
> grant 0.18 0.11 0.03
For just hash+bloom, I'm seeing 1.29 and 0.04.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 15:27 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2024-03-22 15:27 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
Nathan Bossart <[email protected]> writes:
> On Fri, Mar 22, 2024 at 09:47:39AM -0500, Nathan Bossart wrote:
>> hash hash+simd hash+simd+bloom
>> create 1.27 1.27 1.28
>> grant 0.18 0.11 0.03
> For just hash+bloom, I'm seeing 1.29 and 0.04.
Yeah, that's about what I'd expect: hash+bloom ought to remove
most (not quite all) of the opportunity for simd to shine, because
the bloom filter should eliminate most of the list_member_oid calls.
Possibly we could fix that small regression in the create phase
with more careful tuning of the magic constants in the bloom
logic? Although I'd kind of expect that the create step doesn't
ever invoke the bloom filter, else it would have been showing a
performance problem before; so this might not be a good test case
for helping us tune those.
I think remaining questions are:
* Is there any case where the new hash catcache logic could lose
measurably? I kind of doubt it, because we were already computing
the hash value for list searches; so basically the only overhead
is one more palloc per cache during the first list search. (If
you accumulate enough lists to cause a rehash, you're almost
surely well into winning territory.)
* Same question for the bloom logic, but here I think it's mostly
a matter of tuning those constants.
* Do we want to risk back-patching any of this, to fix the performance
regression in v16? I think that the OP's situation is a pretty
narrow one, but maybe he's not the only person who managed to dodge
roles_is_member_of's performance issues in most other cases.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 16:39 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 16:39 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Fri, Mar 22, 2024 at 11:27:46AM -0400, Tom Lane wrote:
> Yeah, that's about what I'd expect: hash+bloom ought to remove
> most (not quite all) of the opportunity for simd to shine, because
> the bloom filter should eliminate most of the list_member_oid calls.
Right. IMHO the SIMD work is still worth considering because there are
probably even more extreme cases where it'll make a decent amount of
difference. Plus, that stuff is pretty low overhead for what you get in
return. That being said, the hash table and Bloom filter should definitely
be the higher priority.
> * Same question for the bloom logic, but here I think it's mostly
> a matter of tuning those constants.
I suspect there might be some regressions just after the point where we
construct the filter, but I'd still expect that to be a reasonable
trade-off. We could probably pretty easily construct some benchmarks to
understand the impact with a given number of roles. (I'm not sure I'll be
able to get to that today.)
> * Do we want to risk back-patching any of this, to fix the performance
> regression in v16? I think that the OP's situation is a pretty
> narrow one, but maybe he's not the only person who managed to dodge
> roles_is_member_of's performance issues in most other cases.
I've heard complaints about performance with many roles before, so I
certainly think this area is worth optimizing. As far as back-patching
goes, my current feeling is that the hash table is probably pretty safe and
provides the majority of the benefit, but anything fancier should probably
be reserved for v17 or v18.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 16:53 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2024-03-22 16:53 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
Nathan Bossart <[email protected]> writes:
> On Fri, Mar 22, 2024 at 11:27:46AM -0400, Tom Lane wrote:
>> * Do we want to risk back-patching any of this, to fix the performance
>> regression in v16? I think that the OP's situation is a pretty
>> narrow one, but maybe he's not the only person who managed to dodge
>> roles_is_member_of's performance issues in most other cases.
> I've heard complaints about performance with many roles before, so I
> certainly think this area is worth optimizing. As far as back-patching
> goes, my current feeling is that the hash table is probably pretty safe and
> provides the majority of the benefit, but anything fancier should probably
> be reserved for v17 or v18.
Yeah. Although both the catcache and list_append_unique_oid bits
are O(N^2), the catcache seems to have a much bigger constant
factor --- when I did a "perf" check on the unpatched code,
I saw catcache eating over 90% of the runtime and list_member_oid
about 2%. So let's fix that part in v16 and call it a day.
It should be safe to back-patch the catcache changes as long as
we put the new fields at the end of the struct and leave cc_lists
present but empty.
Would you like to review the catcache patch further, or do you
think it's good to go?
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 16:54 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 16:54 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Fri, Mar 22, 2024 at 12:53:15PM -0400, Tom Lane wrote:
> Would you like to review the catcache patch further, or do you
> think it's good to go?
I'll take another look this afternoon.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 19:35 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-22 19:35 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Fri, Mar 22, 2024 at 11:54:48AM -0500, Nathan Bossart wrote:
> On Fri, Mar 22, 2024 at 12:53:15PM -0400, Tom Lane wrote:
>> Would you like to review the catcache patch further, or do you
>> think it's good to go?
>
> I'll take another look this afternoon.
LGTM
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-22 20:41 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2024-03-22 20:41 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
Nathan Bossart <[email protected]> writes:
> On Fri, Mar 22, 2024 at 11:54:48AM -0500, Nathan Bossart wrote:
>> On Fri, Mar 22, 2024 at 12:53:15PM -0400, Tom Lane wrote:
>>> Would you like to review the catcache patch further, or do you
>>> think it's good to go?
>> I'll take another look this afternoon.
> LGTM
Thanks for looking, I'll push that shortly.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-25 14:47 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Nathan Bossart @ 2024-03-25 14:47 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Fri, Mar 22, 2024 at 04:41:49PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> LGTM
>
> Thanks for looking, I'll push that shortly.
Are there any changes you'd like to see for the Bloom patch [0]? I'd like
to see about getting that committed for v17. One thing that crossed my
mind is creating a combined list/filter that transparently created a filter
when necessary (for reuse elsewhere), but I'm not sure that's v17 material.
[0] https://postgr.es/m/attachment/158079/bloom_v2.patch
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-25 15:08 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2024-03-25 15:08 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
Nathan Bossart <[email protected]> writes:
> Are there any changes you'd like to see for the Bloom patch [0]? I'd like
> to see about getting that committed for v17. One thing that crossed my
> mind is creating a combined list/filter that transparently created a filter
> when necessary (for reuse elsewhere), but I'm not sure that's v17 material.
Yeah, that thought occurred to me too, but I think we ought to have a
few more use-cases in view before trying to write an API.
As for the patch, I agree it could go into v17, but I think there is
still a little bit of work to do:
* The magic constants (crossover list length and bloom filter size)
need some testing to see if there are better values. They should
probably be made into named #defines, too. I suspect, with little
proof, that the bloom filter size isn't particularly critical --- but
I know we pulled the crossover of 1000 out of thin air, and I have
no certainty that it's even within an order of magnitude of being a
good choice.
* Code needs more than zero comments.
* Is it worth trying to make a subroutine, or at least a macro,
so as not to have 2 copies of the code?
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
@ 2024-03-25 15:16 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Nathan Bossart @ 2024-03-25 15:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; alex work <[email protected]>; Robert Haas <[email protected]>
On Mon, Mar 25, 2024 at 11:08:39AM -0400, Tom Lane wrote:
> * The magic constants (crossover list length and bloom filter size)
> need some testing to see if there are better values. They should
> probably be made into named #defines, too. I suspect, with little
> proof, that the bloom filter size isn't particularly critical --- but
> I know we pulled the crossover of 1000 out of thin air, and I have
> no certainty that it's even within an order of magnitude of being a
> good choice.
I'll try to construct a couple of tests to see if we can determine a proper
order of magnitude.
> * Code needs more than zero comments.
Yup.
> * Is it worth trying to make a subroutine, or at least a macro,
> so as not to have 2 copies of the code?
I think so. I'll try that in the next version.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2024-03-25 15:16 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-07-21 09:05 [PATCH v4 1/2] mark_async_capable(): subpath should match subplan Alexander Pyhalov <[email protected]>
2023-07-21 09:05 [PATCH v4 1/2] mark_async_capable(): subpath should match subplan Alexander Pyhalov <[email protected]>
2024-03-21 20:40 Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 00:51 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 00:59 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[email protected]>
2024-03-22 01:03 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 01:52 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 02:07 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[email protected]>
2024-03-22 02:02 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs alex work <[email protected]>
2024-03-22 14:47 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 14:55 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 15:27 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[email protected]>
2024-03-22 16:39 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 16:53 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[email protected]>
2024-03-22 16:54 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 19:35 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-22 20:41 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[email protected]>
2024-03-25 14:47 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[email protected]>
2024-03-25 15:08 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Tom Lane <[email protected]>
2024-03-25 15:16 ` Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs Nathan Bossart <[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