Horton

Re: index prefetching

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Andres Freund <andres@anarazel.de>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Georgios <gkokolatos@protonmail.com>
Date: 2024-01-12T16:52:53Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. aio: io_uring: Trigger async processing for large IOs

  2. read stream: Split decision about look ahead for AIO and combining

  3. read_stream: Only increase read-ahead distance when waiting for IO

  4. read_stream: Prevent distance from decaying too quickly

  5. Reduce ExecSeqScan* code size using pg_assume()

  6. Fix rare bug in read_stream.c's split IO handling.

  7. Fix multiranges to behave more like dependent types.

  8. Add EXPLAIN (MEMORY) to report planner memory consumption

  9. Optimize nbtree backward scan boundary cases.

  10. Increment xactCompletionCount during subtransaction abort.

  11. Add nbtree Valgrind buffer lock checks.

  12. Add nbtree high key "continuescan" optimization.

  13. Reduce pinning and buffer content locking for btree scans.

  14. Teach btree to handle ScalarArrayOpExpr quals natively.

Not a full response, but just to address a few points:

On Fri, Jan 12, 2024 at 11:42 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> Thinking about this, I think it should be possible to make prefetching
> work even for plans with execute_once=false. In particular, when the
> plan changes direction it should be possible to simply "walk back" the
> prefetch queue, to get to the "correct" place in in the scan. But I'm
> not sure it's worth it, because plans that change direction often can't
> really benefit from prefetches anyway - they'll often visit stuff they
> accessed shortly before anyway. For plans that don't change direction
> but may pause, we don't know if the plan pauses long enough for the
> prefetched pages to get evicted or something. So I think it's OK that
> execute_once=false means no prefetching.

+1.

> > +             * XXX We do add the cache size to the request in order not to
> > +             * have issues with uint64 underflows.
> >
> > I don't know what this means.
> >
>
> There's a check that does this:
>
>       (x + PREFETCH_CACHE_SIZE) >= y
>
> it might also be done as "mathematically equivalent"
>
>       x >= (y - PREFETCH_CACHE_SIZE)
>
> but if the "y" is an uint64, and the value is smaller than the constant,
> this would underflow. It'd eventually disappear, once the "y" gets large
> enough, ofc.

The problem is, I think, that there's no particular reason that
someone reading the existing code should imagine that it might have
been done in that "mathematically equivalent" fashion. I imagined that
you were trying to make a point about adding the cache size to the
request vs. adding nothing, whereas in reality you were trying to make
a point about adding from one side vs. subtracting from the other.

> > +     * We reach here if the index only scan is not parallel, or if we're
> > +     * serially executing an index only scan that was planned to be
> > +     * parallel.
> >
> > Well, this seems sad.
>
> Stale comment, I believe. However, I didn't see much benefits with
> parallel index scan during testing. Having I/O from multiple workers
> generally had the same effect, I think.

Fair point, likely worth mentioning explicitly in the comment.

> Yeah. I renamed all the structs and functions to IndexPrefetchSomething,
> to keep it consistent. And then the constants are all capital, ofc.

It'd still be nice to get table or heap in there, IMHO, but maybe we
can't, and consistency is certainly a good thing regardless of the
details, so thanks for that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com