From: Andres Freund <andres@anarazel.de>
To: Julien Rouhaud <julien.rouhaud@dalibo.com>
Cc: Jeff Janes <jeff.janes@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: Merlin Moncure <mmoncure@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Allow a per-tablespace effective_io_concurrency setting
Date: Wed, 2 Sep 2015 15:53:51 +0200
Message-ID: <20150902135351.GA5286@alap3.anarazel.de> (raw)
In-Reply-To: <55AA27C3.7060009@dalibo.com>
References: <CAHyXU0yaUG9R_E5=1gdXhD-MpWR=Gr=4=EHFD_fRid2+SCQrqA@mail.gmail.com>
<CAHyXU0yiVvfQAnR9cyH=HWh1WbLRsioe=mzRJTHwtr=2azsTdQ@mail.gmail.com>
<20141206130828.GA1314@momjian.us>
<CAHyXU0wgpE2E3B+rmZ959tJT_adPFfPvHNqeA9K9mkJRAT9HXw@mail.gmail.com>
<20141209204337.GC24488@momjian.us>
<CAMkU=1w0nXDcwhyQurh+XCdQ2AyeSLBcMGqrNAYsckLbTqq3mw@mail.gmail.com>
<55AA2469.20306@dalibo.com>
<55AA27C3.7060009@dalibo.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Hi,
On 2015-07-18 12:17:39 +0200, Julien Rouhaud wrote:
> I didn't know that the thread must exists on -hackers to be able to add
> a commitfest entry, so I transfer the thread here.
Please, in the future, also update the title of the thread to something
fitting.
> @@ -539,6 +541,9 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
> {
> BitmapHeapScanState *scanstate;
> Relation currentRelation;
> +#ifdef USE_PREFETCH
> + int new_io_concurrency;
> +#endif
>
> /* check for unsupported flags */
> Assert(!(eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)));
> @@ -598,6 +603,25 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
> */
> currentRelation = ExecOpenScanRelation(estate, node->scan.scanrelid, eflags);
>
> +#ifdef USE_PREFETCH
> + /* check if the effective_io_concurrency has been overloaded for the
> + * tablespace storing the relation and compute the target_prefetch_pages,
> + * or just get the current target_prefetch_pages
> + */
> + new_io_concurrency = get_tablespace_io_concurrency(
> + currentRelation->rd_rel->reltablespace);
> +
> +
> + scanstate->target_prefetch_pages = target_prefetch_pages;
> +
> + if (new_io_concurrency != effective_io_concurrency)
> + {
> + double prefetch_pages;
> + if (compute_io_concurrency(new_io_concurrency, &prefetch_pages))
> + scanstate->target_prefetch_pages = rint(prefetch_pages);
> + }
> +#endif
Maybe it's just me - but imo there should be as few USE_PREFETCH
dependant places in the code as possible. It'll just be 0 when not
supported, that's fine? Especially changing the size of externally
visible structs depending on a configure detected ifdef seems wrong to
me.
> +bool
> +compute_io_concurrency(int io_concurrency, double *target_prefetch_pages)
> +{
> + double new_prefetch_pages = 0.0;
> + int i;
> +
> + /* make sure the io_concurrency value is correct, it may have been forced
> + * with a pg_tablespace UPDATE
> + */
Nitpick: Wrong comment style (/* stands on its own line).
> + if (io_concurrency > MAX_IO_CONCURRENCY)
> + io_concurrency = MAX_IO_CONCURRENCY;
> +
> + /*----------
> + * The user-visible GUC parameter is the number of drives (spindles),
> + * which we need to translate to a number-of-pages-to-prefetch target.
> + * The target value is stashed in *extra and then assigned to the actual
> + * variable by assign_effective_io_concurrency.
> + *
> + * The expected number of prefetch pages needed to keep N drives busy is:
> + *
> + * drives | I/O requests
> + * -------+----------------
> + * 1 | 1
> + * 2 | 2/1 + 2/2 = 3
> + * 3 | 3/1 + 3/2 + 3/3 = 5 1/2
> + * 4 | 4/1 + 4/2 + 4/3 + 4/4 = 8 1/3
> + * n | n * H(n)
I know you just moved this code. But: I don't buy this formula. Like at
all. Doesn't queuing and reordering entirely invalidate the logic here?
Perhaps more relevantly: Imo nodeBitmapHeapscan.c is the wrong place for
this. bufmgr.c maybe?
You also didn't touch
/*
* How many buffers PrefetchBuffer callers should try to stay ahead of their
* ReadBuffer calls by. This is maintained by the assign hook for
* effective_io_concurrency. Zero means "never prefetch".
*/
int target_prefetch_pages = 0;
which surely doesn't make sense anymore after these changes.
But do we even need that variable now?
> diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
> index dc167f9..57008fc 100644
> --- a/src/include/utils/guc.h
> +++ b/src/include/utils/guc.h
> @@ -26,6 +26,9 @@
> #define MAX_KILOBYTES (INT_MAX / 1024)
> #endif
>
> +/* upper limit for effective_io_concurrency */
> +#define MAX_IO_CONCURRENCY 1000
> +
> /*
> * Automatic configuration file name for ALTER SYSTEM.
> * This file will be used to store values of configuration parameters
> @@ -256,6 +259,8 @@ extern int temp_file_limit;
>
> extern int num_temp_buffers;
>
> +extern int effective_io_concurrency;
> +
target_prefetch_pages is declared in bufmgr.h - that seems like a better
place for these.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-performance@postgresql.org
Cc: andres@anarazel.de, julien.rouhaud@dalibo.com, jeff.janes@gmail.com, bruce@momjian.us, mmoncure@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Allow a per-tablespace effective_io_concurrency setting
In-Reply-To: <20150902135351.GA5286@alap3.anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox