Re: Use streaming read API in ANALYZE

On Tue, Sep 10, 2024 at 12:28 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
wrote:

> On Tue, Sep 10, 2024 at 3:36 AM Michael Banck <mbanck(at)gmx(dot)net> wrote:
> > I am a bit confused about the status of this thread. Robert mentioned
> > RC1, so I guess it pertains to v17 but I don't see it on the open item
> > wiki list?
>
> Yes, v17. Alight, I'll add an item.
>
> > Does the above mean you are going to revert it for v17, Thomas? And if
> > so, what exactly? The ANALYZE changes on top of the streaming read API
> > or something else about that API that is being discussed on this thread?
>
> I might have been a little pessimistic in that assessment. Another
> workaround that seems an awful lot cleaner and less invasive would be
> to offer a new ReadStream API function that provides access to block
> numbers and the strategy, ie the arguments of v16's
> scan_analyze_next_block() function. Mats, what do you think about
> this? (I haven't tried to preserve the prefetching behaviour, which
> probably didn't actually too work for you in v16 anyway at a guess,
> I'm just looking for the absolute simplest thing we can do to resolve
> this API mismatch.) TimeScale could then continue to use its v16
> coding to handle the two-relations-in-a-trenchcoat problem, and we
> could continue discussing how to make v18 better.
>

In the original code we could call the methods with an "adjusted" block
number, so the entire logic worked as before because we could just
recursively forward the call with modified parameters. This is a little
different with the new API.

> I looked briefly at another non-heap-like table AM, the Citus Columnar
> TAM. I am not familiar with that code and haven't studied it deeply
> this morning, but its _next_block() currently just returns true, so I
> think it will somehow need to change to counting calls and returning
> false when it thinks its been called enough times (otherwise the loop
> in acquire_sample_rows() won't terminate, I think?). I suppose an
> easy way to do that without generating extra I/O or having to think
> hard about how to preserve the loop cound from v16 would be to use
> this function.
>

Yes, but we are re-using the heapam so forwarding the call to it, which not
only fetches the next block it also reads the buffer. Since you could just
pass in the block number before, it just worked.

As mentioned, we intended to set up a new ReadStream for the "internal"
relation ourselves (I think this is what you mean with "daisy-chain" in the
followup to this mail), but then you need targrows, which is based on
vacattrstats, which is computed with code that is currently either inline
(the loop over the attributes in do_analyze_rel), or static (the
examine_attribute function). We can write our own code for this, it would
help to have the code that does this work callable, or be able to extract
parameters from the existing readstream to at least get a hint. This would
allow us to just get the vacuum attribute stats for an arbitrary relation
and then run the same computations as in do_analyze_rel. Being able to do
the same for the indexes is less important since this is an "internal"
relation and the "public" indexes are the ones that matter.

I attached a tentative patch for this, just doing some refactorings, and
will see if that is sufficient for the current work by trying to use it. (I
thought I would be able to verify this today, but am a little delayed so
I'm sending this anyway.)

A patch like this is a minimal refactoring so should be safe even in an RC.
I have deliberately not tried to do a more serious refactoring although I
see that there are some duplications when doing the same work with the
indexes and it would probably be possible to make a more generic function
for this.

> I think there are broadly three categories of TAMs with respect to
> ANALYZE block sampling: those that are very heap-like (blocks of one
> SMgrRelation) and can just use the stream directly, those that are not
> at all heap-like (doing something completely different to sample
> tuples and ignoring the block aspect but using _next_block() to
> control the loop), and then Timescale's case which is sort of
> somewhere in between: almost heap-like from the point of view of this
> sampling code, ie working with blocks, but fudging the meaning of
> block numbers, which we didn't anticipate.

In this case the block numbers are only from a different relation, so they
are still valid blocks, just encoded in a funny way. The block numbers
trick is just a hack, but the gist is that we want to sample an
arbitrary number of relations/forks when running analysis, not just the
"front-facing" one.

> (I wonder if it fails to
> sample fairly across the underlying relation boundary anyway because
> their data densities must surely be quite different, but that's not
> what we're here to talk about.)
>

Yes, they are, so this is kind-of-a-hack-to-get-it-roughly-correct. The
ideal scenario would be to be able to run the same analysis of that is done
in do_analyze_rel on the "hidden" relation to get an accurate targetrows.
This is what I am trying now with the attached patch.

>
> . o O { We need that wiki page listing TAMs with links to the open
> source ones... }

--
Best wishes,
Mats Kindahl, Timescale