Re: Allow a per-tablespace effective_io_concurrency setting - Mailing list pgsql-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
bythe 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