public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 3/3] POC: Add a pg_hba_matches() function. 10+ messages / 3 participants [nested] [flat]
* [PATCH v5 3/3] POC: Add a pg_hba_matches() function. @ 2022-02-22 13:34 Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Julien Rouhaud @ 2022-02-22 13:34 UTC (permalink / raw) Catversion is bumped. Author: Julien Rouhaud Reviewed-by: FIXME Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud --- src/backend/catalog/system_functions.sql | 9 ++ src/backend/libpq/hba.c | 138 +++++++++++++++++++++++ src/include/catalog/pg_proc.dat | 7 ++ 3 files changed, 154 insertions(+) diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql index 81bac6f581..049cdabc81 100644 --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -594,6 +594,15 @@ LANGUAGE internal STRICT IMMUTABLE PARALLEL SAFE AS 'unicode_is_normalized'; +CREATE OR REPLACE FUNCTION + pg_hba_matches( + IN address inet, IN role text, IN ssl bool DEFAULT false, + OUT file_name text, OUT line_num int4, OUT raw_line text) +RETURNS RECORD +LANGUAGE INTERNAL +VOLATILE +AS 'pg_hba_matches'; + -- -- The default permissions for functions mean that anyone can execute them. -- A number of functions shouldn't be executable by just anyone, but rather diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index 1551b34c53..a757a54c87 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -26,6 +26,7 @@ #include <unistd.h> #include "access/htup_details.h" +#include "catalog/pg_authid.h" #include "catalog/pg_collation.h" #include "catalog/pg_type.h" #include "common/ip.h" @@ -41,6 +42,7 @@ #include "utils/acl.h" #include "utils/builtins.h" #include "utils/guc.h" +#include "utils/inet.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/varlena.h" @@ -2835,3 +2837,139 @@ hba_authname(UserAuth auth_method) return UserAuthName[auth_method]; } + +#define PG_HBA_MATCHES_ATTS 3 + +/* + * SQL-accessible SRF to return the entries that match the given connection + * info, if any. + */ +Datum pg_hba_matches(PG_FUNCTION_ARGS) +{ + MemoryContext ctxt; + inet *address = NULL; + bool ssl_in_use = false; + hbaPort *port = palloc0(sizeof(hbaPort)); + TupleDesc tupdesc; + Datum values[PG_HBA_MATCHES_ATTS]; + bool isnull[PG_HBA_MATCHES_ATTS]; + + if (!is_member_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("only superuser or a member of the pg_read_server_files role may call this function"))); + + if (PG_ARGISNULL(0)) + port->raddr.addr.ss_family = AF_UNIX; + else + { + int bits; + char *ptr; + char tmp[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255/128")]; + + address = PG_GETARG_INET_PP(0); + + bits = ip_maxbits(address) - ip_bits(address); + if (bits != 0) + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("Invalid address"))); + } + + /* force display of max bits, regardless of masklen... */ + if (pg_inet_net_ntop(ip_family(address), ip_addr(address), + ip_maxbits(address), tmp, sizeof(tmp)) == NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION), + errmsg("could not format inet value: %m"))); + + /* Suppress /n if present (shouldn't happen now) */ + if ((ptr = strchr(tmp, '/')) != NULL) + *ptr = '\0'; + + switch (ip_family(address)) + { + case PGSQL_AF_INET: + { + struct sockaddr_in *dst; + + dst = (struct sockaddr_in *) &port->raddr.addr; + dst->sin_family = AF_INET; + + /* ip_addr(address) always contains network representation */ + memcpy(&dst->sin_addr, &ip_addr(address), sizeof(dst->sin_addr)); + + break; + } + /* See pg_inet_net_ntop() for details about those constants */ + case PGSQL_AF_INET6: +#if defined(AF_INET6) && AF_INET6 != PGSQL_AF_INET6 + case AF_INET6: +#endif + { + struct sockaddr_in6 *dst; + + dst = (struct sockaddr_in6 *) &port->raddr.addr; + dst->sin6_family = AF_INET6; + + /* ip_addr(address) always contains network representation */ + memcpy(&dst->sin6_addr, &ip_addr(address), sizeof(dst->sin6_addr)); + + break; + } + default: + elog(ERROR, "unexpected ip_family: %d", ip_family(address)); + break; + } + } + + if (PG_ARGISNULL(1)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("parameter role is mandatory"))); + port->user_name = text_to_cstring(PG_GETARG_TEXT_PP(1)); + + if (!PG_ARGISNULL(2)) + ssl_in_use = PG_GETARG_BOOL(2); + + port->ssl_in_use = ssl_in_use; + + tupdesc = CreateTemplateTupleDesc(PG_HBA_MATCHES_ATTS); + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "file_name", + TEXTOID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 2, "line_num", + INT4OID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 3, "raw_line", + TEXTOID, -1, 0); + + BlessTupleDesc(tupdesc); + + memset(isnull, 0, sizeof(isnull)); + + /* FIXME rework API to not rely on PostmasterContext */ + ctxt = AllocSetContextCreate(CurrentMemoryContext, "load_hba", + ALLOCSET_DEFAULT_SIZES); + PostmasterContext = AllocSetContextCreate(ctxt, + "Postmaster", + ALLOCSET_DEFAULT_SIZES); + parsed_hba_context = NULL; + if (!load_hba()) + ereport(ERROR, + (errcode(ERRCODE_CONFIG_FILE_ERROR), + errmsg("Invalidation auth configuration file"))); + + check_hba(port); + + if (port->hba->auth_method == uaImplicitReject) + PG_RETURN_NULL(); + + values[0] = CStringGetTextDatum(port->hba->sourcefile); + values[1] = Int32GetDatum(port->hba->linenumber); + values[2] = CStringGetTextDatum(port->hba->rawline); + + MemoryContextDelete(PostmasterContext); + PostmasterContext = NULL; + + return HeapTupleGetDatum(heap_form_tuple(tupdesc, values, isnull)); +} diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 04cab629f5..ea7d7c7eff 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6122,6 +6122,13 @@ proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{mapping_number,file_name,line_number,map_name,sys_name,pg_username,error}', prosrc => 'pg_ident_file_mappings' }, +{ oid => '9557', descr => 'show wether the given connection would match an hba line', + proname => 'pg_hba_matches', provolatile => 'v', prorettype => 'record', + proargtypes => 'inet text bool', proisstrict => 'f', + proallargtypes => '{inet,text,bool,text,int4,text}', + proargmodes => '{i,i,i,o,o,o}', + proargnames => '{address,role,ssl,file_name,line_num,raw_line}', + prosrc => 'pg_hba_matches' }, { oid => '1371', descr => 'view system lock information', proname => 'pg_lock_status', prorows => '1000', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', -- 2.33.1 --w64sw4oo7jkfb67p-- ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-02-28 20:06 Melanie Plageman <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Melanie Plageman @ 2024-02-28 20:06 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On Wed, Feb 28, 2024 at 2:23 PM Tomas Vondra <[email protected]> wrote: > > On 2/28/24 15:56, Tomas Vondra wrote: > >> ... > > > > Sure, I can do that. It'll take a couple hours to get the results, I'll > > share them when I have them. > > > > Here are the results with only patches 0001 - 0012 applied (i.e. without > the patch introducing the streaming read API, and the patch switching > the bitmap heap scan to use it). > > The changes in performance don't disappear entirely, but the scale is > certainly much smaller - both in the complete results for all runs, and > for the "optimal" runs that would actually pick bitmapscan. Hmm. I'm trying to think how my refactor could have had this impact. It seems like all the most notable regressions are with 4 parallel workers. What do the numeric column labels mean across the top (2,4,8,16...) -- are they related to "matches"? And if so, what does that mean? - Melanie ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-02-28 23:17 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tomas Vondra @ 2024-02-28 23:17 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On 2/28/24 21:06, Melanie Plageman wrote: > On Wed, Feb 28, 2024 at 2:23 PM Tomas Vondra > <[email protected]> wrote: >> >> On 2/28/24 15:56, Tomas Vondra wrote: >>>> ... >>> >>> Sure, I can do that. It'll take a couple hours to get the results, I'll >>> share them when I have them. >>> >> >> Here are the results with only patches 0001 - 0012 applied (i.e. without >> the patch introducing the streaming read API, and the patch switching >> the bitmap heap scan to use it). >> >> The changes in performance don't disappear entirely, but the scale is >> certainly much smaller - both in the complete results for all runs, and >> for the "optimal" runs that would actually pick bitmapscan. > > Hmm. I'm trying to think how my refactor could have had this impact. > It seems like all the most notable regressions are with 4 parallel > workers. What do the numeric column labels mean across the top > (2,4,8,16...) -- are they related to "matches"? And if so, what does > that mean? > That's the number of distinct values matched by the query, which should be an approximation of the number of matching rows. The number of distinct values in the data set differs by data set, but for 1M rows it's roughly like this: uniform: 10k linear: 10k cyclic: 100 So for example matches=128 means ~1% of rows for uniform/linear, and 100% for cyclic data sets. As for the possible cause, I think it's clear most of the difference comes from the last patch that actually switches bitmap heap scan to the streaming read API. That's mostly expected/understandable, although we probably need to look into the regressions or cases with e_i_c=0. To analyze the 0001-0012 patches, maybe it'd be helpful to run tests for individual patches. I can try doing that tomorrow. It'll have to be a limited set of tests, to reduce the time, but might tell us whether it's due to a single patch or multiple patches. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-02-28 23:40 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Melanie Plageman @ 2024-02-28 23:40 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On Wed, Feb 28, 2024 at 6:17 PM Tomas Vondra <[email protected]> wrote: > > > > On 2/28/24 21:06, Melanie Plageman wrote: > > On Wed, Feb 28, 2024 at 2:23 PM Tomas Vondra > > <[email protected]> wrote: > >> > >> On 2/28/24 15:56, Tomas Vondra wrote: > >>>> ... > >>> > >>> Sure, I can do that. It'll take a couple hours to get the results, I'll > >>> share them when I have them. > >>> > >> > >> Here are the results with only patches 0001 - 0012 applied (i.e. without > >> the patch introducing the streaming read API, and the patch switching > >> the bitmap heap scan to use it). > >> > >> The changes in performance don't disappear entirely, but the scale is > >> certainly much smaller - both in the complete results for all runs, and > >> for the "optimal" runs that would actually pick bitmapscan. > > > > Hmm. I'm trying to think how my refactor could have had this impact. > > It seems like all the most notable regressions are with 4 parallel > > workers. What do the numeric column labels mean across the top > > (2,4,8,16...) -- are they related to "matches"? And if so, what does > > that mean? > > > > That's the number of distinct values matched by the query, which should > be an approximation of the number of matching rows. The number of > distinct values in the data set differs by data set, but for 1M rows > it's roughly like this: > > uniform: 10k > linear: 10k > cyclic: 100 > > So for example matches=128 means ~1% of rows for uniform/linear, and > 100% for cyclic data sets. Ah, thank you for the explanation. I also looked at your script after having sent this email and saw that it is clear in your script what "matches" is. > As for the possible cause, I think it's clear most of the difference > comes from the last patch that actually switches bitmap heap scan to the > streaming read API. That's mostly expected/understandable, although we > probably need to look into the regressions or cases with e_i_c=0. Right, I'm mostly surprised about the regressions for patches 0001-0012. Re eic 0: Thomas Munro and I chatted off-list, and you bring up a great point about eic 0. In old bitmapheapscan code eic 0 basically disabled prefetching but with the streaming read API, it will still issue fadvises when eic is 0. That is an easy one line fix. Thomas prefers to fix it by always avoiding an fadvise for the last buffer in a range before issuing a read (since we are about to read it anyway, best not fadvise it too). This will fix eic 0 and also cut one system call from each invocation of the streaming read machinery. > To analyze the 0001-0012 patches, maybe it'd be helpful to run tests for > individual patches. I can try doing that tomorrow. It'll have to be a > limited set of tests, to reduce the time, but might tell us whether it's > due to a single patch or multiple patches. Yes, tomorrow I planned to start trying to repro some of the "red" cases myself. Any one of the commits could cause a slight regression but a 3.5x regression is quite surprising, so I might focus on trying to repro that locally and then narrow down which patch causes it. For the non-cached regressions, perhaps the commit to use the correct recheck flag (0004) when prefetching could be the culprit. And for the cached regressions, my money is on the commit which changes the whole control flow of BitmapHeapNext() and the next_block() and next_tuple() functions (0010). - Melanie ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-02-29 12:54 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tomas Vondra @ 2024-02-29 12:54 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On 2/29/24 00:40, Melanie Plageman wrote: > On Wed, Feb 28, 2024 at 6:17 PM Tomas Vondra > <[email protected]> wrote: >> >> >> >> On 2/28/24 21:06, Melanie Plageman wrote: >>> On Wed, Feb 28, 2024 at 2:23 PM Tomas Vondra >>> <[email protected]> wrote: >>>> >>>> On 2/28/24 15:56, Tomas Vondra wrote: >>>>>> ... >>>>> >>>>> Sure, I can do that. It'll take a couple hours to get the results, I'll >>>>> share them when I have them. >>>>> >>>> >>>> Here are the results with only patches 0001 - 0012 applied (i.e. without >>>> the patch introducing the streaming read API, and the patch switching >>>> the bitmap heap scan to use it). >>>> >>>> The changes in performance don't disappear entirely, but the scale is >>>> certainly much smaller - both in the complete results for all runs, and >>>> for the "optimal" runs that would actually pick bitmapscan. >>> >>> Hmm. I'm trying to think how my refactor could have had this impact. >>> It seems like all the most notable regressions are with 4 parallel >>> workers. What do the numeric column labels mean across the top >>> (2,4,8,16...) -- are they related to "matches"? And if so, what does >>> that mean? >>> >> >> That's the number of distinct values matched by the query, which should >> be an approximation of the number of matching rows. The number of >> distinct values in the data set differs by data set, but for 1M rows >> it's roughly like this: >> >> uniform: 10k >> linear: 10k >> cyclic: 100 >> >> So for example matches=128 means ~1% of rows for uniform/linear, and >> 100% for cyclic data sets. > > Ah, thank you for the explanation. I also looked at your script after > having sent this email and saw that it is clear in your script what > "matches" is. > >> As for the possible cause, I think it's clear most of the difference >> comes from the last patch that actually switches bitmap heap scan to the >> streaming read API. That's mostly expected/understandable, although we >> probably need to look into the regressions or cases with e_i_c=0. > > Right, I'm mostly surprised about the regressions for patches 0001-0012. > > Re eic 0: Thomas Munro and I chatted off-list, and you bring up a > great point about eic 0. In old bitmapheapscan code eic 0 basically > disabled prefetching but with the streaming read API, it will still > issue fadvises when eic is 0. That is an easy one line fix. Thomas > prefers to fix it by always avoiding an fadvise for the last buffer in > a range before issuing a read (since we are about to read it anyway, > best not fadvise it too). This will fix eic 0 and also cut one system > call from each invocation of the streaming read machinery. > >> To analyze the 0001-0012 patches, maybe it'd be helpful to run tests for >> individual patches. I can try doing that tomorrow. It'll have to be a >> limited set of tests, to reduce the time, but might tell us whether it's >> due to a single patch or multiple patches. > > Yes, tomorrow I planned to start trying to repro some of the "red" > cases myself. Any one of the commits could cause a slight regression > but a 3.5x regression is quite surprising, so I might focus on trying > to repro that locally and then narrow down which patch causes it. > > For the non-cached regressions, perhaps the commit to use the correct > recheck flag (0004) when prefetching could be the culprit. And for the > cached regressions, my money is on the commit which changes the whole > control flow of BitmapHeapNext() and the next_block() and next_tuple() > functions (0010). > I do have some partial results, comparing the patches. I only ran one of the more affected workloads (cyclic) on the xeon, attached is a PDF comparing master and the 0001-0014 patches. The percentages are timing vs. the preceding patch (green - faster, red - slower). This suggests only patches 0010 and 0014 affect performance, the rest is just noise. I'll see if I can do more runs and get data from the other machine (seems it's more significant on old SATA SSDs). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Attachments: [application/pdf] patch-comparison.pdf (155.3K, ../../[email protected]/2-patch-comparison.pdf) download ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-02-29 21:19 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Melanie Plageman @ 2024-02-29 21:19 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On Thu, Feb 29, 2024 at 7:54 AM Tomas Vondra <[email protected]> wrote: > > > > On 2/29/24 00:40, Melanie Plageman wrote: > > On Wed, Feb 28, 2024 at 6:17 PM Tomas Vondra > > <[email protected]> wrote: > >> > >> > >> > >> On 2/28/24 21:06, Melanie Plageman wrote: > >>> On Wed, Feb 28, 2024 at 2:23 PM Tomas Vondra > >>> <[email protected]> wrote: > >>>> > >>>> On 2/28/24 15:56, Tomas Vondra wrote: > >>>>>> ... > >>>>> > >>>>> Sure, I can do that. It'll take a couple hours to get the results, I'll > >>>>> share them when I have them. > >>>>> > >>>> > >>>> Here are the results with only patches 0001 - 0012 applied (i.e. without > >>>> the patch introducing the streaming read API, and the patch switching > >>>> the bitmap heap scan to use it). > >>>> > >>>> The changes in performance don't disappear entirely, but the scale is > >>>> certainly much smaller - both in the complete results for all runs, and > >>>> for the "optimal" runs that would actually pick bitmapscan. > >>> > >>> Hmm. I'm trying to think how my refactor could have had this impact. > >>> It seems like all the most notable regressions are with 4 parallel > >>> workers. What do the numeric column labels mean across the top > >>> (2,4,8,16...) -- are they related to "matches"? And if so, what does > >>> that mean? > >>> > >> > >> That's the number of distinct values matched by the query, which should > >> be an approximation of the number of matching rows. The number of > >> distinct values in the data set differs by data set, but for 1M rows > >> it's roughly like this: > >> > >> uniform: 10k > >> linear: 10k > >> cyclic: 100 > >> > >> So for example matches=128 means ~1% of rows for uniform/linear, and > >> 100% for cyclic data sets. > > > > Ah, thank you for the explanation. I also looked at your script after > > having sent this email and saw that it is clear in your script what > > "matches" is. > > > >> As for the possible cause, I think it's clear most of the difference > >> comes from the last patch that actually switches bitmap heap scan to the > >> streaming read API. That's mostly expected/understandable, although we > >> probably need to look into the regressions or cases with e_i_c=0. > > > > Right, I'm mostly surprised about the regressions for patches 0001-0012. > > > > Re eic 0: Thomas Munro and I chatted off-list, and you bring up a > > great point about eic 0. In old bitmapheapscan code eic 0 basically > > disabled prefetching but with the streaming read API, it will still > > issue fadvises when eic is 0. That is an easy one line fix. Thomas > > prefers to fix it by always avoiding an fadvise for the last buffer in > > a range before issuing a read (since we are about to read it anyway, > > best not fadvise it too). This will fix eic 0 and also cut one system > > call from each invocation of the streaming read machinery. > > > >> To analyze the 0001-0012 patches, maybe it'd be helpful to run tests for > >> individual patches. I can try doing that tomorrow. It'll have to be a > >> limited set of tests, to reduce the time, but might tell us whether it's > >> due to a single patch or multiple patches. > > > > Yes, tomorrow I planned to start trying to repro some of the "red" > > cases myself. Any one of the commits could cause a slight regression > > but a 3.5x regression is quite surprising, so I might focus on trying > > to repro that locally and then narrow down which patch causes it. > > > > For the non-cached regressions, perhaps the commit to use the correct > > recheck flag (0004) when prefetching could be the culprit. And for the > > cached regressions, my money is on the commit which changes the whole > > control flow of BitmapHeapNext() and the next_block() and next_tuple() > > functions (0010). > > > > I do have some partial results, comparing the patches. I only ran one of > the more affected workloads (cyclic) on the xeon, attached is a PDF > comparing master and the 0001-0014 patches. The percentages are timing > vs. the preceding patch (green - faster, red - slower). Just confirming: the results are for uncached? - Melanie ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-02-29 22:44 Tomas Vondra <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Tomas Vondra @ 2024-02-29 22:44 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On 2/29/24 22:19, Melanie Plageman wrote: > On Thu, Feb 29, 2024 at 7:54 AM Tomas Vondra > <[email protected]> wrote: >> >> >> >> On 2/29/24 00:40, Melanie Plageman wrote: >>> On Wed, Feb 28, 2024 at 6:17 PM Tomas Vondra >>> <[email protected]> wrote: >>>> >>>> >>>> >>>> On 2/28/24 21:06, Melanie Plageman wrote: >>>>> On Wed, Feb 28, 2024 at 2:23 PM Tomas Vondra >>>>> <[email protected]> wrote: >>>>>> >>>>>> On 2/28/24 15:56, Tomas Vondra wrote: >>>>>>>> ... >>>>>>> >>>>>>> Sure, I can do that. It'll take a couple hours to get the results, I'll >>>>>>> share them when I have them. >>>>>>> >>>>>> >>>>>> Here are the results with only patches 0001 - 0012 applied (i.e. without >>>>>> the patch introducing the streaming read API, and the patch switching >>>>>> the bitmap heap scan to use it). >>>>>> >>>>>> The changes in performance don't disappear entirely, but the scale is >>>>>> certainly much smaller - both in the complete results for all runs, and >>>>>> for the "optimal" runs that would actually pick bitmapscan. >>>>> >>>>> Hmm. I'm trying to think how my refactor could have had this impact. >>>>> It seems like all the most notable regressions are with 4 parallel >>>>> workers. What do the numeric column labels mean across the top >>>>> (2,4,8,16...) -- are they related to "matches"? And if so, what does >>>>> that mean? >>>>> >>>> >>>> That's the number of distinct values matched by the query, which should >>>> be an approximation of the number of matching rows. The number of >>>> distinct values in the data set differs by data set, but for 1M rows >>>> it's roughly like this: >>>> >>>> uniform: 10k >>>> linear: 10k >>>> cyclic: 100 >>>> >>>> So for example matches=128 means ~1% of rows for uniform/linear, and >>>> 100% for cyclic data sets. >>> >>> Ah, thank you for the explanation. I also looked at your script after >>> having sent this email and saw that it is clear in your script what >>> "matches" is. >>> >>>> As for the possible cause, I think it's clear most of the difference >>>> comes from the last patch that actually switches bitmap heap scan to the >>>> streaming read API. That's mostly expected/understandable, although we >>>> probably need to look into the regressions or cases with e_i_c=0. >>> >>> Right, I'm mostly surprised about the regressions for patches 0001-0012. >>> >>> Re eic 0: Thomas Munro and I chatted off-list, and you bring up a >>> great point about eic 0. In old bitmapheapscan code eic 0 basically >>> disabled prefetching but with the streaming read API, it will still >>> issue fadvises when eic is 0. That is an easy one line fix. Thomas >>> prefers to fix it by always avoiding an fadvise for the last buffer in >>> a range before issuing a read (since we are about to read it anyway, >>> best not fadvise it too). This will fix eic 0 and also cut one system >>> call from each invocation of the streaming read machinery. >>> >>>> To analyze the 0001-0012 patches, maybe it'd be helpful to run tests for >>>> individual patches. I can try doing that tomorrow. It'll have to be a >>>> limited set of tests, to reduce the time, but might tell us whether it's >>>> due to a single patch or multiple patches. >>> >>> Yes, tomorrow I planned to start trying to repro some of the "red" >>> cases myself. Any one of the commits could cause a slight regression >>> but a 3.5x regression is quite surprising, so I might focus on trying >>> to repro that locally and then narrow down which patch causes it. >>> >>> For the non-cached regressions, perhaps the commit to use the correct >>> recheck flag (0004) when prefetching could be the culprit. And for the >>> cached regressions, my money is on the commit which changes the whole >>> control flow of BitmapHeapNext() and the next_block() and next_tuple() >>> functions (0010). >>> >> >> I do have some partial results, comparing the patches. I only ran one of >> the more affected workloads (cyclic) on the xeon, attached is a PDF >> comparing master and the 0001-0014 patches. The percentages are timing >> vs. the preceding patch (green - faster, red - slower). > > Just confirming: the results are for uncached? > Yes, cyclic data set, uncached case. I picked this because it seemed like one of the most affected cases. Do you want me to test some other cases too? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-02-29 23:44 Tomas Vondra <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Tomas Vondra @ 2024-02-29 23:44 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On 2/29/24 23:44, Tomas Vondra wrote: > > ... > >>> >>> I do have some partial results, comparing the patches. I only ran one of >>> the more affected workloads (cyclic) on the xeon, attached is a PDF >>> comparing master and the 0001-0014 patches. The percentages are timing >>> vs. the preceding patch (green - faster, red - slower). >> >> Just confirming: the results are for uncached? >> > > Yes, cyclic data set, uncached case. I picked this because it seemed > like one of the most affected cases. Do you want me to test some other > cases too? > BTW I decided to look at the data from a slightly different angle and compare the behavior with increasing effective_io_concurrency. Attached are charts for three "uncached" cases: * uniform, work_mem=4MB, workers_per_gather=0 * linear-fuzz, work_mem=4MB, workers_per_gather=0 * uniform, work_mem=4MB, workers_per_gather=4 Each page has charts for master and patched build (with all patches). I think there's a pretty obvious difference in how increasing e_i_c affects the two builds: 1) On master there's clear difference between eic=0 and eic=1 cases, but on the patched build there's literally no difference - for example the "uniform" distribution is clearly not great for prefetching, but eic=0 regresses to eic=1 poor behavior). Note: This is where the the "red bands" in the charts come from. 2) For some reason, the prefetching with eic>1 perform much better with the patches, except for with very low selectivity values (close to 0%). Not sure why this is happening - either the overhead is much lower (which would matter on these "adversarial" data distribution, but how could that be when fadvise is not free), or it ends up not doing any prefetching (but then what about (1)?). 3) I'm not sure about the linear-fuzz case, the only explanation I have we're able to skip almost all of the prefetches (and read-ahead likely works pretty well here). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Attachments: [application/pdf] prefetch-charts.pdf (389.3K, ../../[email protected]/2-prefetch-charts.pdf) download ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-03-01 00:29 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Melanie Plageman @ 2024-03-01 00:29 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On Thu, Feb 29, 2024 at 5:44 PM Tomas Vondra <[email protected]> wrote: > > > > On 2/29/24 22:19, Melanie Plageman wrote: > > On Thu, Feb 29, 2024 at 7:54 AM Tomas Vondra > > <[email protected]> wrote: > >> > >> > >> > >> On 2/29/24 00:40, Melanie Plageman wrote: > >>> On Wed, Feb 28, 2024 at 6:17 PM Tomas Vondra > >>> <[email protected]> wrote: > >>>> > >>>> > >>>> > >>>> On 2/28/24 21:06, Melanie Plageman wrote: > >>>>> On Wed, Feb 28, 2024 at 2:23 PM Tomas Vondra > >>>>> <[email protected]> wrote: > >>>>>> > >>>>>> On 2/28/24 15:56, Tomas Vondra wrote: > >>>>>>>> ... > >>>>>>> > >>>>>>> Sure, I can do that. It'll take a couple hours to get the results, I'll > >>>>>>> share them when I have them. > >>>>>>> > >>>>>> > >>>>>> Here are the results with only patches 0001 - 0012 applied (i.e. without > >>>>>> the patch introducing the streaming read API, and the patch switching > >>>>>> the bitmap heap scan to use it). > >>>>>> > >>>>>> The changes in performance don't disappear entirely, but the scale is > >>>>>> certainly much smaller - both in the complete results for all runs, and > >>>>>> for the "optimal" runs that would actually pick bitmapscan. > >>>>> > >>>>> Hmm. I'm trying to think how my refactor could have had this impact. > >>>>> It seems like all the most notable regressions are with 4 parallel > >>>>> workers. What do the numeric column labels mean across the top > >>>>> (2,4,8,16...) -- are they related to "matches"? And if so, what does > >>>>> that mean? > >>>>> > >>>> > >>>> That's the number of distinct values matched by the query, which should > >>>> be an approximation of the number of matching rows. The number of > >>>> distinct values in the data set differs by data set, but for 1M rows > >>>> it's roughly like this: > >>>> > >>>> uniform: 10k > >>>> linear: 10k > >>>> cyclic: 100 > >>>> > >>>> So for example matches=128 means ~1% of rows for uniform/linear, and > >>>> 100% for cyclic data sets. > >>> > >>> Ah, thank you for the explanation. I also looked at your script after > >>> having sent this email and saw that it is clear in your script what > >>> "matches" is. > >>> > >>>> As for the possible cause, I think it's clear most of the difference > >>>> comes from the last patch that actually switches bitmap heap scan to the > >>>> streaming read API. That's mostly expected/understandable, although we > >>>> probably need to look into the regressions or cases with e_i_c=0. > >>> > >>> Right, I'm mostly surprised about the regressions for patches 0001-0012. > >>> > >>> Re eic 0: Thomas Munro and I chatted off-list, and you bring up a > >>> great point about eic 0. In old bitmapheapscan code eic 0 basically > >>> disabled prefetching but with the streaming read API, it will still > >>> issue fadvises when eic is 0. That is an easy one line fix. Thomas > >>> prefers to fix it by always avoiding an fadvise for the last buffer in > >>> a range before issuing a read (since we are about to read it anyway, > >>> best not fadvise it too). This will fix eic 0 and also cut one system > >>> call from each invocation of the streaming read machinery. > >>> > >>>> To analyze the 0001-0012 patches, maybe it'd be helpful to run tests for > >>>> individual patches. I can try doing that tomorrow. It'll have to be a > >>>> limited set of tests, to reduce the time, but might tell us whether it's > >>>> due to a single patch or multiple patches. > >>> > >>> Yes, tomorrow I planned to start trying to repro some of the "red" > >>> cases myself. Any one of the commits could cause a slight regression > >>> but a 3.5x regression is quite surprising, so I might focus on trying > >>> to repro that locally and then narrow down which patch causes it. > >>> > >>> For the non-cached regressions, perhaps the commit to use the correct > >>> recheck flag (0004) when prefetching could be the culprit. And for the > >>> cached regressions, my money is on the commit which changes the whole > >>> control flow of BitmapHeapNext() and the next_block() and next_tuple() > >>> functions (0010). > >>> > >> > >> I do have some partial results, comparing the patches. I only ran one of > >> the more affected workloads (cyclic) on the xeon, attached is a PDF > >> comparing master and the 0001-0014 patches. The percentages are timing > >> vs. the preceding patch (green - faster, red - slower). > > > > Just confirming: the results are for uncached? > > > > Yes, cyclic data set, uncached case. I picked this because it seemed > like one of the most affected cases. Do you want me to test some other > cases too? So, I actually may have found the source of at least part of the regression with 0010. I was able to reproduce the regression with patch 0010 applied for the unached case with 4 workers and eic 8 and 100000000 rows for the cyclic dataset. I see it for all number of matches. The regression went away (for this specific example) when I moved the BitmapAdjustPrefetchIterator call back up to before the call to table_scan_bitmap_next_block() like this: diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index f7ecc060317..268996bdeea 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -279,6 +279,8 @@ BitmapHeapNext(BitmapHeapScanState *node) } new_page: + BitmapAdjustPrefetchIterator(node, node->blockno); + if (!table_scan_bitmap_next_block(scan, &node->recheck, &lossy, &node->blockno)) break; @@ -287,7 +289,6 @@ new_page: else node->exact_pages++; - BitmapAdjustPrefetchIterator(node, node->blockno); /* Adjust the prefetch target */ BitmapAdjustPrefetchTarget(node); } It makes sense this would fix it. I haven't tried all the combinations you tried. Do you mind running your tests with the new code? I've pushed it into this branch. https://github.com/melanieplageman/postgres/commits/bhs_pgsr/ Note that this will fix none of the issues with 0014 because that has removed all of the old prefetching code anyway. Thank you sooo much for running these to begin with and then helping me figure out what is going on! - Melanie ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BitmapHeapScan streaming read user and prelim refactoring @ 2024-03-01 01:18 Melanie Plageman <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Melanie Plageman @ 2024-03-01 01:18 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Nazir Bilal Yavuz <[email protected]> On Thu, Feb 29, 2024 at 6:44 PM Tomas Vondra <[email protected]> wrote: > > On 2/29/24 23:44, Tomas Vondra wrote: > > > > ... > > > >>> > >>> I do have some partial results, comparing the patches. I only ran one of > >>> the more affected workloads (cyclic) on the xeon, attached is a PDF > >>> comparing master and the 0001-0014 patches. The percentages are timing > >>> vs. the preceding patch (green - faster, red - slower). > >> > >> Just confirming: the results are for uncached? > >> > > > > Yes, cyclic data set, uncached case. I picked this because it seemed > > like one of the most affected cases. Do you want me to test some other > > cases too? > > > > BTW I decided to look at the data from a slightly different angle and > compare the behavior with increasing effective_io_concurrency. Attached > are charts for three "uncached" cases: > > * uniform, work_mem=4MB, workers_per_gather=0 > * linear-fuzz, work_mem=4MB, workers_per_gather=0 > * uniform, work_mem=4MB, workers_per_gather=4 > > Each page has charts for master and patched build (with all patches). I > think there's a pretty obvious difference in how increasing e_i_c > affects the two builds: Wow! These visualizations make it exceptionally clear. I want to go to the Vondra school of data visualizations for performance results! > 1) On master there's clear difference between eic=0 and eic=1 cases, but > on the patched build there's literally no difference - for example the > "uniform" distribution is clearly not great for prefetching, but eic=0 > regresses to eic=1 poor behavior). Yes, so eic=0 and eic=1 are identical with the streaming read API. That is, eic 0 does not disable prefetching. Thomas is going to update the streaming read API to avoid issuing an fadvise for the last block in a range before issuing a read -- which would mean no prefetching with eic 0 and eic 1. Not doing prefetching with eic 1 actually seems like the right behavior -- which would be different than what master is doing, right? Hopefully this fixes the clear difference between master and the patched version at eic 0. > 2) For some reason, the prefetching with eic>1 perform much better with > the patches, except for with very low selectivity values (close to 0%). > Not sure why this is happening - either the overhead is much lower > (which would matter on these "adversarial" data distribution, but how > could that be when fadvise is not free), or it ends up not doing any > prefetching (but then what about (1)?). For the uniform with four parallel workers, eic == 0 being worse than master makes sense for the above reason. But I'm not totally sure why eic == 1 would be worse with the patch than with master. Both are doing a (somewhat useless) prefetch. With very low selectivity, you are less likely to get readahead (right?) and similarly less likely to be able to build up > 8kB IOs -- which is one of the main value propositions of the streaming read code. I imagine that this larger read benefit is part of why the performance is better at higher selectivities with the patch. This might be a silly experiment, but we could try decreasing MAX_BUFFERS_PER_TRANSFER on the patched version and see if the performance gains go away. > 3) I'm not sure about the linear-fuzz case, the only explanation I have > we're able to skip almost all of the prefetches (and read-ahead likely > works pretty well here). I started looking at the data generated by linear-fuzz to understand exactly what effect the fuzz was having but haven't had time to really understand the characteristics of this dataset. In the original results, I thought uncached linear-fuzz and linear had similar results (performance improvement from master). What do you expect with linear vs linear-fuzz? - Melanie ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-03-01 01:18 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-02-22 13:34 [PATCH v5 3/3] POC: Add a pg_hba_matches() function. Julien Rouhaud <[email protected]> 2024-02-28 20:06 Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-02-28 23:17 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-02-28 23:40 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-02-29 12:54 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-02-29 21:19 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-02-29 22:44 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-02-29 23:44 ` Re: BitmapHeapScan streaming read user and prelim refactoring Tomas Vondra <[email protected]> 2024-03-01 01:18 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[email protected]> 2024-03-01 00:29 ` Re: BitmapHeapScan streaming read user and prelim refactoring Melanie Plageman <[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