Re: Use streaming read API in ANALYZE

On Sat, Aug 24, 2024 at 5:31 AM Thomas Munro <thomas.munro@gmail.com> wrote:

> On Thu, Aug 22, 2024 at 7:31 PM Mats Kindahl <mats@timescale.com> wrote:
> > The alternate version proposed by Nazir allows you to deide which
> interface to use.
> > Reverting the patch entirely would also solve the problem.
>

After digging through the code a little more I discovered that
there actually is another one: move the ReadStream struct into
read_stream.h.


> > Passing down the block sampler and the strategy to scan_begin() and move
> the ReadStream setup in analyze.c into initscan() in heapam.c, but this
> requires adding new parameters to this function.
> > Having accessors that allow you to get the block sampler and strategy
> from the ReadStream object.
>
> I'm a bit confused about how it can make sense to use the same
> BlockSampler with a side relation/fork.  Could you point me at the
> code?
>

Sorry, that was a bit unclear. Intention was not to re-use the block
sampler but to set a new one up with parameters from the original block
sampler, which would require access to it. (The strategy is less of a
problem since only one is used.)

To elaborate on the situation:

For the TAM in question we have two different storage areas, both are
heaps. Both relations use the same attributes "publicly" (they are
internally different, but we transform them to look the same). One of the
relations is the "default" one and is stored in rd_rel. In order to run
ANALYZE, we need to sample blocks from both relations, in slightly
different ways.

With the old interface, we faked the number of blocks in relation_size()
callback and claimed that there were N + M blocks. When then being asked
about a block by block number, we could easily pick the correct relation
and just forward the call.

With the new ReadStream API, a read-stream is (automatically) set up on the
"default" relation, but we can set up a separate read-stream inside the TAM
for the other relation. However, the difficulty is in setting it up
correctly:

We cannot use the "fake number of block"-trick since the read stream does
not only compute the block number, but actually tries to read the buffer in
the relation provided when setting up the read stream, so a block number
outside the range of this relation will not be found since it is in a
different relation.

If we could create our own read stream with both relations, that could be
solved and we could just implement the same logic, but direct it to the
correct relations depending on where we want to read the block. Unless I am
mistaken, there is already support for this since there is an array of
in-progress I/O and it would be trivial to extend this with more
relations+forks, if you have access to the structure definition. The
ReadStream struct is, however, an opaque struct so it's hard to hack around
with it. Just making the struct declaration public would potentially solve
a lot of problems here. (See attached patch, which is close to the minimum
of what is needed to allow extension writers to tweak the contents.)

Since both relations are using the same attributes with the same
"analyzability", having that information would be useful to compute the
targrows for setting up the additional stream, but it is computed in
do_analyze_rel() and not further propagated, so it needs to be re-computed
if we want to set up a separate read-stream.


> > It would be great if this could be fixed before the PG17 release now
> that 27bc1772fc8 was reverted.
>
> Ack.  Thinking...
>

Right now I think that just making the ReadStream struct available in the
header file is the best approach. It is a safe and low-risk fix (so
something that can be added to a beta) and will allow extension writers to
hack to their hearts' contents. In addition to that, being able to select
what interface to use would also help.



> Random thought: is there a wiki page or something where we can find
> out about all the table AM projects?  For the successor to
> 27bc1772fc8, I hope they'll be following along.
>

At this point, unfortunately not, we are quite early in this. Once I have
something, I'll share.
-- 
Best wishes,
Mats Kindahl, Timescale