public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v2 1/1] remove db_user_namespace 14+ messages / 4 participants [nested] [flat]
* [PATCH v2 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/client-auth.sgml | 5 -- doc/src/sgml/config.sgml | 52 ------------------- src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 9 ---- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - 8 files changed, 105 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 204d09df67..6c95f0df1e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1253,11 +1253,6 @@ omicron bryanh guest1 attacks. </para> - <para> - The <literal>md5</literal> method cannot be used with - the <xref linkend="guc-db-user-namespace"/> feature. - </para> - <para> To ease transition from the <literal>md5</literal> method to the newer SCRAM method, if <literal>md5</literal> is specified as a method diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 4c49393fc5..33a13fdf32 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - /* * Truncate given database and user names to length of a Postgres name. * This avoids lookup failures when overlength names are given. diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 71e27f8eb0..25d9008bb6 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1534,15 +1534,6 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, - { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL - }, - &Db_user_namespace, - false, - NULL, NULL, NULL - }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple -- 2.25.1 --X1bOJ3K7DJ5YkBrT-- ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-05-17 23:30 Thomas Munro <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Thomas Munro @ 2024-05-17 23:30 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers On Sat, May 18, 2024 at 8:09 AM Thomas Munro <[email protected]> wrote: > On Sat, May 18, 2024 at 1:00 AM Alexander Lakhin <[email protected]> wrote: > > I decided to compare v17 vs v16 performance (as I did the last year [1]) > > and discovered that v17 loses to v16 in the pg_tpcds (s64da_tpcds) > > benchmark, query15 (and several others, but I focused on this one): > > Best pg-src-master--.* worse than pg-src-16--.* by 52.2 percents (229.84 > 151.03): pg_tpcds.query15 > > Average pg-src-master--.* worse than pg-src-16--.* by 53.4 percents (234.20 > 152.64): pg_tpcds.query15 > > Please look at the full html report attached in case you're interested. > > > > (I used my pg-mark tool to measure/analyze performance, but I believe the > > same results can be seen without it.) > > Will investigate, but if it's easy for you to rerun, does it help if > you increase Linux readahead, eg blockdev --setra setting? Andres happened to have TPC-DS handy, and reproduced that regression in q15. We tried some stuff and figured out that it requires parallel_leader_participation=on, ie that this looks like some kind of parallel fairness and/or timing problem. It seems to be a question of which worker finishes up processing matching rows, and the leader gets a ~10ms head start but may be a little more greedy with the new streaming code. He tried reordering the table contents and then saw 17 beat 16. So for q15, initial indications are that this isn't a fundamental regression, it's just a test that is sensitive to some arbitrary conditions. I'll try to figure out some more details about that, ie is it being too greedy on small-ish tables, and generally I do wonder about the interactions between the heuristics and batching working at different levels (OS, seq scan, read stream, hence my earlier ra question which is likely a red herring) and how there might be unintended consequences/interference patterns, but this particular case seems more data dependent. ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-05-18 04:47 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 2 replies; 14+ messages in thread From: Thomas Munro @ 2024-05-18 04:47 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers On Sat, May 18, 2024 at 11:30 AM Thomas Munro <[email protected]> wrote: > Andres happened to have TPC-DS handy, and reproduced that regression > in q15. We tried some stuff and figured out that it requires > parallel_leader_participation=on, ie that this looks like some kind of > parallel fairness and/or timing problem. It seems to be a question of > which worker finishes up processing matching rows, and the leader gets > a ~10ms head start but may be a little more greedy with the new > streaming code. He tried reordering the table contents and then saw > 17 beat 16. So for q15, initial indications are that this isn't a > fundamental regression, it's just a test that is sensitive to some > arbitrary conditions. > > I'll try to figure out some more details about that, ie is it being > too greedy on small-ish tables, After more debugging, we learned a lot more things... 1. That query produces spectacularly bad estimates, so we finish up having to increase the number of buckets in a parallel hash join many times. That is quite interesting, but unrelated to new code. 2. Parallel hash join is quite slow at negotiating an increase in the number of hash bucket, if all of the input tuples are being filtered out by quals, because of the choice of where workers check for PHJ_GROWTH_NEED_MORE_BUCKETS. That could be improved quite easily I think. I have put that on my todo list 'cause that's also my code, but it's not a new issue it's just one that is now highlighted... 3. This bit of read_stream.c is exacerbating unfairness in the underlying scan, so that 1 and 2 come together and produce a nasty slowdown, which goes away if you change it like so: - BlockNumber blocknums[16]; + BlockNumber blocknums[1]; I will follow up after some more study. ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-05-18 19:00 Alexander Lakhin <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Alexander Lakhin @ 2024-05-18 19:00 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers Hello Thomas, 18.05.2024 07:47, Thomas Munro wrote: > After more debugging, we learned a lot more things... > > 1. That query produces spectacularly bad estimates, so we finish up > having to increase the number of buckets in a parallel hash join many > times. That is quite interesting, but unrelated to new code. > 2. Parallel hash join is quite slow at negotiating an increase in the > number of hash bucket, if all of the input tuples are being filtered > out by quals, because of the choice of where workers check for > PHJ_GROWTH_NEED_MORE_BUCKETS. That could be improved quite easily I > think. I have put that on my todo list 'cause that's also my code, > but it's not a new issue it's just one that is now highlighted... > 3. This bit of read_stream.c is exacerbating unfairness in the > underlying scan, so that 1 and 2 come together and produce a nasty > slowdown, which goes away if you change it like so: > > - BlockNumber blocknums[16]; > + BlockNumber blocknums[1]; > > I will follow up after some more study. Thank you for the information! Unfortunately, I can't see significant differences in my environment with parallel_leader_participation=off. With blocknums[1], timing is changed, but the effect is not persistent. 10 query15 executions in a row, b7b0f3f27: 277.932 ms 281.805 ms 278.335 ms 281.565 ms 284.167 ms 283.171 ms 281.165 ms 281.615 ms 285.394 ms 277.301 ms b7b0f3f27~1: 159.789 ms 165.407 ms 160.893 ms 159.343 ms 160.936 ms 161.577 ms 161.637 ms 163.421 ms 163.143 ms 167.109 ms b7b0f3f27 + blocknums[1]: 164.133 ms 280.920 ms 160.748 ms 163.182 ms 161.709 ms 161.998 ms 161.239 ms 276.256 ms 161.601 ms 160.384 ms I placed PGDATA on tmpfs to rule out any blockdev specifics (increasing blockdev ra from 256 to 4096 didn't help me with PGDATA on NVME either.) Best regards, Alexander ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-05-18 23:47 Thomas Munro <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Thomas Munro @ 2024-05-18 23:47 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers On Sun, May 19, 2024 at 7:00 AM Alexander Lakhin <[email protected]> wrote: > With blocknums[1], timing is changed, but the effect is not persistent. > 10 query15 executions in a row, b7b0f3f27: > 277.932 ms > 281.805 ms > 278.335 ms > 281.565 ms > 284.167 ms > 283.171 ms > 281.165 ms > 281.615 ms > 285.394 ms > 277.301 ms The bad time 10/10. > b7b0f3f27~1: > 159.789 ms > 165.407 ms > 160.893 ms > 159.343 ms > 160.936 ms > 161.577 ms > 161.637 ms > 163.421 ms > 163.143 ms > 167.109 ms The good time 10/10. > b7b0f3f27 + blocknums[1]: > 164.133 ms > 280.920 ms > 160.748 ms > 163.182 ms > 161.709 ms > 161.998 ms > 161.239 ms > 276.256 ms > 161.601 ms > 160.384 ms The good time 8/10, the bad time 2/10. Thanks for checking! I bet all branches can show that flip/flop instability in these adverse conditions, depending on random scheduling details. I will start a new thread with a patch for the root cause of that, ie problem #2 (this will need back-patching), and post a fix for #3 (v17 blocknums[N] tweak affecting fairness/likelihood, which was probably basically a bit of ill-advised premature optimisation) here in a few days. ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-05-20 21:10 Melanie Plageman <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Melanie Plageman @ 2024-05-20 21:10 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers Thank you to all of you for looking into this. On Sat, May 18, 2024 at 12:47 AM Thomas Munro <[email protected]> wrote: > > On Sat, May 18, 2024 at 11:30 AM Thomas Munro <[email protected]> wrote: > > Andres happened to have TPC-DS handy, and reproduced that regression > > in q15. We tried some stuff and figured out that it requires > > parallel_leader_participation=on, ie that this looks like some kind of > > parallel fairness and/or timing problem. It seems to be a question of > > which worker finishes up processing matching rows, and the leader gets > > a ~10ms head start but may be a little more greedy with the new > > streaming code. He tried reordering the table contents and then saw > > 17 beat 16. So for q15, initial indications are that this isn't a > > fundamental regression, it's just a test that is sensitive to some > > arbitrary conditions. > > > > I'll try to figure out some more details about that, ie is it being > > too greedy on small-ish tables, > > After more debugging, we learned a lot more things... > > 1. That query produces spectacularly bad estimates, so we finish up > having to increase the number of buckets in a parallel hash join many > times. That is quite interesting, but unrelated to new code. > 2. Parallel hash join is quite slow at negotiating an increase in the > number of hash bucket, if all of the input tuples are being filtered > out by quals, because of the choice of where workers check for > PHJ_GROWTH_NEED_MORE_BUCKETS. That could be improved quite easily I > think. I have put that on my todo list 'cause that's also my code, > but it's not a new issue it's just one that is now highlighted... > 3. This bit of read_stream.c is exacerbating unfairness in the > underlying scan, so that 1 and 2 come together and produce a nasty > slowdown, which goes away if you change it like so: > > - BlockNumber blocknums[16]; > + BlockNumber blocknums[1]; > > I will follow up after some more study. So, if you are seeing the slow-down mostly go away by reducing blocknums array size, does the regression only appear when the scan data is fully in shared buffers? Or is this blocknums other use (dealing with short reads)? Is your theory that one worker ends up reading 16 blocks that should have been distributed across multiple workers? - Melanie ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-05-20 21:34 Thomas Munro <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Thomas Munro @ 2024-05-20 21:34 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers On Tue, May 21, 2024 at 9:11 AM Melanie Plageman <[email protected]> wrote: > So, if you are seeing the slow-down mostly go away by reducing > blocknums array size, does the regression only appear when the scan > data is fully in shared buffers? Or is this blocknums other use > (dealing with short reads)? That must be true (that blocknums array is normally only "filled" in the "fast path", where all buffers are found in cache). > Is your theory that one worker ends up reading 16 blocks that should > have been distributed across multiple workers? Yes, it just jiggles the odds around a bit, introducing a bit of extra unfairness by calling the callback in a tighter loop to build a little batch, revealing a pre-existing problem. The mistake in PHJ (problem #2 above) is that, once a worker decides it would like all workers to stop inserting so it can increase the number of buckets, it sets a flag to ask them to do that, and waits for them to see it, but if there is a worker filtering all tuples out, it never checks the "growth" flag. So it scans all the way to the end while the other guy waits. Normally it checks that flag when it is time to allocate a new chunk of memory, which seemed to make sense to me at the time: if we've hit the needs-more-buckets (or needs-more-batches) logic, then surely workers are inserting tuples and will soon allocate a new chunk! But, of course, here is the edge case where that isn't true: we had bad estimates so hash table too small (problem #1), we got lots of tuples clustered over a few heap pages and decided to expand the hash table, but right at that moment, matching tuples ran out so somebody had to finish the whole scan without ever checking the flag (problem #2), and that someone happened to have all the rest of the pages because we made the lookahead a bit less fair (problem #3). Nice confluence of problems. I expect #2 and #3 to be easy to fix, and I didn't look at the estimation problem #1 at all (perhaps a stats puzzle designed by the TPC to trip us up?). ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-08-27 06:52 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Thomas Munro @ 2024-08-27 06:52 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers Here's a really simple way to see the new unfairness at the end of a parallel scan: drop table if exists t; create table t (i int); insert into t select generate_series(1, 100000); alter table t set (parallel_workers = 2); set parallel_setup_cost = 0; set parallel_leader_participation = off; explain (analyze, buffers, verbose) select count(*) from t; On my machine, unpatched master shows: Worker 0: actual time=0.036..12.452 rows=51076 loops=1 Buffers: shared hit=226 Worker 1: actual time=0.037..12.003 rows=48924 loops=1 Buffers: shared hit=217 The attached patch, which I'd like to push, is effectively what Alexander tested (blocknums[16] -> blocknums[1]). There's no point in using an array of size 1, so I've turned it into a simple variable and deleted the relevant comments. My machine shows: Worker 0: actual time=0.038..12.115 rows=49946 loops=1 Buffers: shared hit=221 Worker 1: actual time=0.038..12.109 rows=50054 loops=1 Buffers: shared hit=222 That difference may not seem huge, but other pre-existing things are going pathologically wrong in the reported query that magnify it (see my earlier analysis). It's an interesting problem that will require more study (my earlier analysis missed a detail that I'll write about separately), but it doesn't seem to be new or have easy fixes, so that will have to be for later work. Attachments: [text/x-patch] 0001-Fix-unfairness-in-all-cached-parallel-seq-scan.patch (5.7K, ../../CA+hUKGKHpN7Gg7x_0ALz1e2ZguS1CjrPioDLB0CHjjv7zpiWeg@mail.gmail.com/2-0001-Fix-unfairness-in-all-cached-parallel-seq-scan.patch) download | inline diff: From 07ff31ad30bf9e383e42336e28143852e3793c5b Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Tue, 27 Aug 2024 15:11:53 +1200 Subject: [PATCH] Fix unfairness in all-cached parallel seq scan. Commit b5a9b18c introduced block streaming infrastructure with a special fast path for all-cached blocks, and commit b7b0f3f2 connected the infrastructure up to sequential scans. One of the fast path optimizations had an unintended consequence: it interfered with the underlying parallel sequential scan's block allocator, which has its own ramp-up and ramp-down algorithm. A scan of a small all-cached table could give more blocks to one worker. In some plans (probably already very bad plans, such as the one reported by Alexander), the unfairness could be magnified. Now all-cached scans will call the next-block-number callback just once each time it wants a new block, instead of trying to buffer 16 block numbers at once. Back-patch to 17. Reported-by: Alexander Lakhin <[email protected]> Discussion: https://postgr.es/m/63a63690-dd92-c809-0b47-af05459e95d1%40gmail.com --- src/backend/storage/aio/read_stream.c | 82 ++++++++------------------- 1 file changed, 24 insertions(+), 58 deletions(-) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index a83c18c2a4b..57d9e93c001 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -117,13 +117,10 @@ struct ReadStream bool advice_enabled; /* - * Small buffer of block numbers, useful for 'ungetting' to resolve flow - * control problems when I/Os are split. Also useful for batch-loading - * block numbers in the fast path. + * One-block buffer to support 'ungetting' a block number, to resolve flow + * control problems when I/Os are split. */ - BlockNumber blocknums[16]; - int16 blocknums_count; - int16 blocknums_next; + BlockNumber buffered_blocknum; /* * The callback that will tell us which block numbers to read, and an @@ -167,68 +164,39 @@ get_per_buffer_data(ReadStream *stream, int16 buffer_index) } /* - * Ask the callback which block it would like us to read next, with a small - * buffer in front to allow read_stream_unget_block() to work and to allow the - * fast path to skip this function and work directly from the array. + * Ask the callback which block it would like us to read next, with a one-block + * buffer in front to allow read_stream_unget_block() to work. */ static inline BlockNumber read_stream_get_block(ReadStream *stream, void *per_buffer_data) { - if (stream->blocknums_next < stream->blocknums_count) - return stream->blocknums[stream->blocknums_next++]; + BlockNumber blocknum; - /* - * We only bother to fetch one at a time here (but see the fast path which - * uses more). - */ - return stream->callback(stream, - stream->callback_private_data, - per_buffer_data); + blocknum = stream->buffered_blocknum; + if (blocknum != InvalidBlockNumber) + stream->buffered_blocknum = InvalidBlockNumber; + else + blocknum = stream->callback(stream, + stream->callback_private_data, + per_buffer_data); + + return blocknum; } /* * In order to deal with short reads in StartReadBuffers(), we sometimes need - * to defer handling of a block until later. + * to defer handling of a block number we've already received from the callback + * until later. */ static inline void read_stream_unget_block(ReadStream *stream, BlockNumber blocknum) { - if (stream->blocknums_next == stream->blocknums_count) - { - /* Never initialized or entirely consumed. Re-initialize. */ - stream->blocknums[0] = blocknum; - stream->blocknums_count = 1; - stream->blocknums_next = 0; - } - else - { - /* Must be the last value return from blocknums array. */ - Assert(stream->blocknums_next > 0); - stream->blocknums_next--; - Assert(stream->blocknums[stream->blocknums_next] == blocknum); - } + /* We shouldn't ever unget more than one block. */ + Assert(stream->buffered_blocknum == InvalidBlockNumber); + Assert(blocknum != InvalidBlockNumber); + stream->buffered_blocknum = blocknum; } -#ifndef READ_STREAM_DISABLE_FAST_PATH -static void -read_stream_fill_blocknums(ReadStream *stream) -{ - BlockNumber blocknum; - int i = 0; - - do - { - blocknum = stream->callback(stream, - stream->callback_private_data, - NULL); - stream->blocknums[i++] = blocknum; - } while (i < lengthof(stream->blocknums) && - blocknum != InvalidBlockNumber); - stream->blocknums_count = i; - stream->blocknums_next = 0; -} -#endif - static void read_stream_start_pending_read(ReadStream *stream, bool suppress_advice) { @@ -530,6 +498,7 @@ read_stream_begin_impl(int flags, stream->queue_size = queue_size; stream->callback = callback; stream->callback_private_data = callback_private_data; + stream->buffered_blocknum = InvalidBlockNumber; /* * Skip the initial ramp-up phase if the caller says we're going to be @@ -649,9 +618,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) Assert(buffer != InvalidBuffer); /* Choose the next block to pin. */ - if (unlikely(stream->blocknums_next == stream->blocknums_count)) - read_stream_fill_blocknums(stream); - next_blocknum = stream->blocknums[stream->blocknums_next++]; + next_blocknum = read_stream_get_block(stream, NULL); if (likely(next_blocknum != InvalidBlockNumber)) { @@ -828,8 +795,7 @@ read_stream_reset(ReadStream *stream) stream->distance = 0; /* Forget buffered block numbers and fast path state. */ - stream->blocknums_next = 0; - stream->blocknums_count = 0; + stream->buffered_blocknum = InvalidBlockNumber; stream->fast_path = false; /* Unpin anything that wasn't consumed. */ -- 2.46.0 ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-08-27 13:00 Alexander Lakhin <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Alexander Lakhin @ 2024-08-27 13:00 UTC (permalink / raw) To: Thomas Munro <[email protected]>; Melanie Plageman <[email protected]>; +Cc: Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers Hello Thomas, 27.08.2024 09:52, Thomas Munro wrote: > Here's a really simple way to see the new unfairness at the end of a > parallel scan: > > drop table if exists t; > create table t (i int); > insert into t select generate_series(1, 100000); > alter table t set (parallel_workers = 2); > set parallel_setup_cost = 0; > set parallel_leader_participation = off; > explain (analyze, buffers, verbose) select count(*) from t; > > On my machine, unpatched master shows: > > Worker 0: actual time=0.036..12.452 rows=51076 loops=1 > Buffers: shared hit=226 > Worker 1: actual time=0.037..12.003 rows=48924 loops=1 > Buffers: shared hit=217 > > The attached patch, which I'd like to push, is effectively what > Alexander tested (blocknums[16] -> blocknums[1]). There's no point in > using an array of size 1, so I've turned it into a simple variable and > deleted the relevant comments. My machine shows: > > Worker 0: actual time=0.038..12.115 rows=49946 loops=1 > Buffers: shared hit=221 > Worker 1: actual time=0.038..12.109 rows=50054 loops=1 > Buffers: shared hit=222 > > That difference may not seem huge, but other pre-existing things are > going pathologically wrong in the reported query that magnify it (see > my earlier analysis). It's an interesting problem that will require > more study (my earlier analysis missed a detail that I'll write about > separately), but it doesn't seem to be new or have easy fixes, so that > will have to be for later work. I've tried your query and could not get sustainable results, unfortunately. The following script: rm -rf "$PGDATA"; initdb -D "$PGDATA" >initdb.log 2>&1 pg_ctl -s -l server.log start cat << EOF | psql | grep 'Parallel Seq Scan' -A10 | grep 'Worker ' -A1 create table t (i int); insert into t select generate_series(1, 100000); alter table t set (parallel_workers = 2); set parallel_setup_cost = 0; set parallel_leader_participation = off; explain (analyze, buffers, verbose) select count(*) from t; EOF pg_ctl -s stop gives me unstable numbers on unpatched master: Worker 0: actual time=0.024..5.814 rows=51076 loops=1 Buffers: shared hit=226 Worker 1: actual time=0.023..5.614 rows=48924 loops=1 Buffers: shared hit=217 --- Worker 0: actual time=0.027..5.130 rows=36612 loops=1 Buffers: shared hit=162 Worker 1: actual time=0.013..5.605 rows=63388 loops=1 Buffers: shared hit=281 --- Worker 0: actual time=0.025..5.447 rows=47460 loops=1 Buffers: shared hit=210 Worker 1: actual time=0.019..5.688 rows=52540 loops=1 Buffers: shared hit=233 and also with the patch applied: Worker 0: actual time=0.012..4.486 rows=55478 loops=1 Buffers: shared hit=246 Worker 1: actual time=0.014..4.430 rows=44522 loops=1 Buffers: shared hit=197 --- Worker 0: actual time=0.013..4.269 rows=55822 loops=1 Buffers: shared hit=247 Worker 1: actual time=0.017..4.238 rows=44178 loops=1 Buffers: shared hit=196 --- Worker 0: actual time=0.014..4.974 rows=50624 loops=1 Buffers: shared hit=224 Worker 1: actual time=0.016..4.932 rows=49376 loops=1 Buffers: shared hit=219 --- Worker 0: actual time=0.012..5.459 rows=65648 loops=1 Buffers: shared hit=291 Worker 1: actual time=0.022..5.109 rows=34352 loops=1 Buffers: shared hit=152 Please correct me, if I'm doing something wrong. Best regards, Alexander ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-08-28 04:45 Thomas Munro <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Thomas Munro @ 2024-08-28 04:45 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers On Wed, Aug 28, 2024 at 1:00 AM Alexander Lakhin <[email protected]> wrote: > gives me unstable numbers on unpatched master: > Buffers: shared hit=226 > Buffers: shared hit=217 > Buffers: shared hit=162 > Buffers: shared hit=281 > Buffers: shared hit=210 > Buffers: shared hit=233 > and also with the patch applied: > Buffers: shared hit=246 > Buffers: shared hit=197 > Buffers: shared hit=247 > Buffers: shared hit=196 > Buffers: shared hit=224 > Buffers: shared hit=219 > Buffers: shared hit=291 > Buffers: shared hit=152 > > Please correct me, if I'm doing something wrong. Huh. I can reproduce what I showed with pretty low variance, on my FreeBSD, Linux and macOS systems. I included parallel_leader_participation=off so that the workers would hopefully start as closely together in time as possible, and hopefully allow only a block or so of variation in the outcome. If I do: create or replace function f(i int) returns int language plpgsql parallel safe as $$ begin raise notice '% pid %', clock_timestamp(), pg_backend_pid(); return i; end; $$; then postgres=# explain (analyze) select f(i) from t limit 1; NOTICE: 2024-08-28 16:41:32.845747+12 pid 47019 NOTICE: 2024-08-28 16:41:32.845746+12 pid 47018 shows start times differ by only a few microseconds at most, often 0. I wonder if your system is more variable at beginning execution? Maybe you're on a multi-socket system and forking/startup times vary in some way I can't see on small systems or something like that? ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-08-28 09:00 Alexander Lakhin <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Alexander Lakhin @ 2024-08-28 09:00 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers 28.08.2024 07:45, Thomas Munro wrote: > Huh. I can reproduce what I showed with pretty low variance, on my > FreeBSD, Linux and macOS systems. I included > parallel_leader_participation=off so that the workers would hopefully > start as closely together in time as possible, and hopefully allow > only a block or so of variation in the outcome. If I do: > > create or replace function f(i int) returns int language plpgsql > parallel safe as $$ > begin > raise notice '% pid %', clock_timestamp(), pg_backend_pid(); > return i; > end; > $$; > > then > > postgres=# explain (analyze) select f(i) from t limit 1; > NOTICE: 2024-08-28 16:41:32.845747+12 pid 47019 > NOTICE: 2024-08-28 16:41:32.845746+12 pid 47018 > > shows start times differ by only a few microseconds at most, often 0. > I wonder if your system is more variable at beginning execution? > Maybe you're on a multi-socket system and forking/startup times vary > in some way I can't see on small systems or something like that? I use a single-socket system with AMD Ryzen 5900X, running Ubuntu 20.04. With no active background tasks running, I'm seeing: NOTICE: 2024-08-28 08:17:36.917162+00 pid 320103 NOTICE: 2024-08-28 08:17:36.917163+00 pid 320102 NOTICE: 2024-08-28 08:17:40.592333+00 pid 320143 NOTICE: 2024-08-28 08:17:40.592645+00 pid 320144 With log_min_messages = DEBUG3, I get: NOTICE: 2024-08-28 08:41:59.309364+00 pid 338263 NOTICE: 2024-08-28 08:41:59.310079+00 pid 338264 And the following messages in the server.log: 2024-08-28 08:41:59.304 UTC [338251] DEBUG: starting background worker process "parallel worker for PID 338262" 2024-08-28 08:41:59.304 UTC [338251] DEBUG: starting background worker process "parallel worker for PID 338262" 2024-08-28 08:41:59.305 UTC [338263] DEBUG: InitPostgres 2024-08-28 08:41:59.305 UTC [338264] DEBUG: InitPostgres 2024-08-28 08:41:59.309 UTC [338263] NOTICE: 2024-08-28 08:41:59.309364+00 pid 338263 2024-08-28 08:41:59.309 UTC [338263] CONTEXT: PL/pgSQL function f(integer) line 3 at RAISE 2024-08-28 08:41:59.309 UTC [338262] NOTICE: 2024-08-28 08:41:59.309364+00 pid 338263 2024-08-28 08:41:59.309 UTC [338262] CONTEXT: PL/pgSQL function f(integer) line 3 at RAISE parallel worker 2024-08-28 08:41:59.309 UTC [338263] DEBUG: shmem_exit(0): 5 before_shmem_exit callbacks to make 2024-08-28 08:41:59.309 UTC [338263] DEBUG: shmem_exit(0): 6 on_shmem_exit callbacks to make 2024-08-28 08:41:59.309 UTC [338263] DEBUG: proc_exit(0): 1 callbacks to make 2024-08-28 08:41:59.309 UTC [338263] DEBUG: exit(0) 2024-08-28 08:41:59.309 UTC [338263] DEBUG: shmem_exit(-1): 0 before_shmem_exit callbacks to make 2024-08-28 08:41:59.309 UTC [338263] DEBUG: shmem_exit(-1): 0 on_shmem_exit callbacks to make 2024-08-28 08:41:59.309 UTC [338263] DEBUG: proc_exit(-1): 0 callbacks to make 2024-08-28 08:41:59.310 UTC [338264] NOTICE: 2024-08-28 08:41:59.310079+00 pid 338264 2024-08-28 08:41:59.310 UTC [338264] CONTEXT: PL/pgSQL function f(integer) line 3 at RAISE It looks like the two parallel workers were started simultaneously, but then the second one lagged behind... Best regards, Alexander ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-08-28 22:16 Thomas Munro <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Thomas Munro @ 2024-08-28 22:16 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers On Wed, Aug 28, 2024 at 9:00 PM Alexander Lakhin <[email protected]> wrote: > 2024-08-28 08:41:59.304 UTC [338251] DEBUG: starting background worker process "parallel worker for PID 338262" > 2024-08-28 08:41:59.304 UTC [338251] DEBUG: starting background worker process "parallel worker for PID 338262" > 2024-08-28 08:41:59.305 UTC [338263] DEBUG: InitPostgres > 2024-08-28 08:41:59.305 UTC [338264] DEBUG: InitPostgres > 2024-08-28 08:41:59.309 UTC [338263] NOTICE: 2024-08-28 08:41:59.309364+00 pid 338263 > 2024-08-28 08:41:59.310 UTC [338264] NOTICE: 2024-08-28 08:41:59.310079+00 pid 338264 > It looks like the two parallel workers were started simultaneously, but > then the second one lagged behind... Yeah. That's quite interesting, and must destabilise that simple-minded demo. I'm curious to know exactly what contention is causing that (about 3/4 of a millisecond that I don't see and now I want to know what it's waiting for), but it's a very crude test lacking timer resolution in the earlier messages, and it's an unrelated topic and a distraction. Perhaps it explains why you saw two different behaviours in Q15 with the patch and I didn't, though. Really it shouldn't be so sensitive to such variations, it's obviously a terrible plan, and TPC-DS needs a planner hacker mega-brain applied to it; I'm going to try to nerd-snipe one... ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-08-29 13:00 Alexander Lakhin <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Alexander Lakhin @ 2024-08-29 13:00 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers Hello Thomas, 29.08.2024 01:16, Thomas Munro wrote: > Yeah. That's quite interesting, and must destabilise that > simple-minded demo. I'm curious to know exactly what contention is > causing that (about 3/4 of a millisecond that I don't see and now I > want to know what it's waiting for), but it's a very crude test > lacking timer resolution in the earlier messages, and it's an > unrelated topic and a distraction. Perhaps it explains why you saw > two different behaviours in Q15 with the patch and I didn't, though. > Really it shouldn't be so sensitive to such variations, it's obviously > a terrible plan, and TPC-DS needs a planner hacker mega-brain applied > to it; I'm going to try to nerd-snipe one... I looked at two perf profiles of such out-of-sync processes and found no extra calls or whatsoever in the slow one, it just has the number of perf samples increased proportionally. It made me suspect CPU frequency scaling... Indeed, with the "performance" governor set and the boost mode disabled, I'm now seeing much more stable numbers (I do this tuning before running performance tests, but I had forgotten about that when I ran that your test, my bad). I'm sorry for the noise and the distraction. Still, now I can confirm your results. Without the patch, two parallel workers gave "Buffers: shared hit=217 / Buffers: shared hit=226" 10 times out of 10. And with the patch, I've got "shared hit=219 / shared hit=224" 3 times, "shared hit=220 / shared hit=223" 4 times, "shared hit=221 / shared hit=222" 3 times of 10. On b7b0f3f27~1, my results are: "shared hit=219 / shared hit=224": 2 "shared hit=220 / shared hit=223": 3 "shared hit=221 / shared hit=222": 4 "shared hit=218 / shared hit=225": 1 Best regards, Alexander ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Streaming read-ready sequential scan code @ 2024-08-31 05:38 Thomas Munro <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Thomas Munro @ 2024-08-31 05:38 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers On Fri, Aug 30, 2024 at 1:00 AM Alexander Lakhin <[email protected]> wrote: > I looked at two perf profiles of such out-of-sync processes and found no > extra calls or whatsoever in the slow one, it just has the number of perf > samples increased proportionally. It made me suspect CPU frequency > scaling... Indeed, with the "performance" governor set and the boost mode > disabled, I'm now seeing much more stable numbers (I do this tuning before > running performance tests, but I had forgotten about that when I ran that > your test, my bad). Aha, mystery solved. I have pushed the fix. Thanks! ^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2024-08-31 05:38 UTC | newest] Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-06-30 19:46 [PATCH v2 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2024-05-17 23:30 Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]> 2024-05-18 04:47 ` Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]> 2024-05-18 19:00 ` Re: Streaming read-ready sequential scan code Alexander Lakhin <[email protected]> 2024-05-18 23:47 ` Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]> 2024-05-20 21:10 ` Re: Streaming read-ready sequential scan code Melanie Plageman <[email protected]> 2024-05-20 21:34 ` Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]> 2024-08-27 06:52 ` Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]> 2024-08-27 13:00 ` Re: Streaming read-ready sequential scan code Alexander Lakhin <[email protected]> 2024-08-28 04:45 ` Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]> 2024-08-28 09:00 ` Re: Streaming read-ready sequential scan code Alexander Lakhin <[email protected]> 2024-08-28 22:16 ` Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]> 2024-08-29 13:00 ` Re: Streaming read-ready sequential scan code Alexander Lakhin <[email protected]> 2024-08-31 05:38 ` Re: Streaming read-ready sequential scan code Thomas Munro <[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