Re: Make ringbuffer threshold and ringbuffer sizes configurable?

Hi,

On 2020-02-06 07:18:00 +0100, Laurenz Albe wrote:
> On Wed, 2020-02-05 at 20:00 -0800, Andres Freund wrote:
> > The ringbuffers we use for seqscans, vacuum, copy etc can cause very
> > drastic slowdowns (see e.g. [1]), an can cause some workloads to
> > practically never end up utilizing shared buffers. ETL workloads
> > e.g. regularly fight with that problem.
> > 
> > I think it would make sense to have seqscan_ringbuffer_threshold,
> > {bulkread,bulkwrite,vacuum}_ringbuffer_size. I think they often sensibly
> > are set in proportion of shared_buffers, so I suggest defining them as
> > floats, where negative values divide shared_buffers, whereas positive
> > values are absolute sizes, and 0 disables the use of ringbuffers.
> 
> Sounds reasonable.

> I feel that it should be as few GUCs as possible, so I think that
> having one per type of operation might be too granular.

They already are set to different sizes, so I don't really see how
that's something doable. The relevant bits of code are:

BufferAccessStrategy
GetAccessStrategy(BufferAccessStrategyType btype)
{
    BufferAccessStrategy strategy;
    int            ring_size;

    /*
     * Select ring size to use.  See buffer/README for rationales.
     *
     * Note: if you change the ring size for BAS_BULKREAD, see also
     * SYNC_SCAN_REPORT_INTERVAL in access/heap/syncscan.c.
     */
    switch (btype)
    {
        case BAS_NORMAL:
            /* if someone asks for NORMAL, just give 'em a "default" object */
            return NULL;

        case BAS_BULKREAD:
            ring_size = 256 * 1024 / BLCKSZ;
            break;
        case BAS_BULKWRITE:
            ring_size = 16 * 1024 * 1024 / BLCKSZ;
            break;
        case BAS_VACUUM:
            ring_size = 256 * 1024 / BLCKSZ;
            break;

and


    /*
     * If the table is large relative to NBuffers, use a bulk-read access
     * strategy and enable synchronized scanning (see syncscan.c).  Although
     * the thresholds for these features could be different, we make them the
     * same so that there are only two behaviors to tune rather than four.
     * (However, some callers need to be able to disable one or both of these
     * behaviors, independently of the size of the table; also there is a GUC
     * variable that can disable synchronized scanning.)
     *
     * Note that table_block_parallelscan_initialize has a very similar test;
     * if you change this, consider changing that one, too.
     */
    if (!RelationUsesLocalBuffers(scan->rs_base.rs_rd) &&
        scan->rs_nblocks > NBuffers / 4)
    {
        allow_strat = (scan->rs_base.rs_flags & SO_ALLOW_STRAT) != 0;
        allow_sync = (scan->rs_base.rs_flags & SO_ALLOW_SYNC) != 0;
    }
    else
        allow_strat = allow_sync = false;


> This should of course also be a storage parameter that can be
> set per table.

I honestly don't quite get that. What precisely is this addressing? I
mean fine, I can add that, but it's sufficiently more complicated than
the GUCs, and I don't really forsee that being particularly useful to
tune on a per table basis. A lot of the reason behind having the
ringbuffers is about managing the whole system impact, rather than
making individual table fast/slow.

- Andres