Re: Allow a per-tablespace effective_io_concurrency setting - Mailing list pgsql-hackers

Hi,

Please find attached a v2 of the patch. See below for changes.


On 02/09/2015 15:53, Andres Freund wrote:
>
> 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.
>

I removed these ifdefs, and the more problematic one in the struct.

>> +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?
>

Changing the formula, or changing the GUC to a number of pages to
prefetch is still discussed, so no change here.

> Perhaps more relevantly: Imo nodeBitmapHeapscan.c is the wrong place for
> this. bufmgr.c maybe?
>

Moved to bufmgr.c


> 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?

I slighty updated the comment.  If the table doesn't belong to a
tablespace with an overloaded effective_io_concurrency, keeping this
pre-computed target_prefetch_pages can save a few cycles on each
execution, so I think it's better to keep it.

>
>> 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.
>

Moved to bufmgr.h


As said in a previous mail, I also fixed a problem when having settings
other than effective_io_concurrency for a tablespace lead to ignore the
regular effective_io_concurrency.

I also added the forgotten lock level (AccessExclusiveLock) for this
tablespace setting, which was leading to a failed assert during initdb.

Regards.

--
Julien Rouhaud
http://dalibo.com - http://dalibo.org