Subject: Re: Use streaming read API in ANALYZE

From: Robert Haas <[email-masked]>
Date: Wed, 4 Sep 2024 11:36:21 -0400

On Wed, Sep 4, 2024 at 6:38 AM Thomas Munro <[email-masked]> wrote:
> Thanks for the explanation.  I think we should revert it.  IMHO it was
> a nice clean example of a streaming transformation, but unfortunately
> it transformed an API that nobody liked in the first place, and broke
> some weird and wonderful workarounds.  Let's try again in 18.

The problem I have with this is that we just released RC1. I suppose
if we have to make this change it's better to do it sooner than later,
but are we sure we want to whack this around this close to final
release?

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

---

From: Thomas Munro <[email-masked]>
Date: Thu, 5 Sep 2024 11:34:21 +1200

> but are we sure we want to whack this around this close to final

I hear you.  But I definitely don't want to (and likely can't at this
point) make any of the other proposed changes, and I also don't want
to break Timescale.  That seems to leave only one option: go back to
the v16 API for RC2, and hope that the ongoing table AM discussions
for v18 (CF #4866) will fix all the problems for the people whose TAMs
don't quack like a "heap", and the people whose TAMs do and who would
not like to duplicate the code, and the people who want streaming I/O.

---

From: Mats Kindahl <[email-masked]>
Date: Thu, 5 Sep 2024 08:45:18 +0200

> not like to duplicate the code, and the people who want streaming I/O.

Forgive me for asking, but I am not entirely sure why the ReadStream struct
is opaque. The usual reasons are:

   - You want to provide an ABI to allow extensions to work with new major
   versions without re-compiling. Right now it is necessary to recompile
   extensions anyway, this does not seem to apply. (Because there are a lot of
   other changes that you need when switching versions because of the lack of
   a stable ABI for other parts of the code. However, it might be that the
   goal is to support it eventually, and then it would make sense to start
   making structs opaque.)
   - You want to ensure that you can make modifications *inside* a major
   version without breaking ABIs and requiring a re-compile. In this case, you
   could still follow safe practice of adding new fields last, not relying on
   the size of the struct for anything (e.g., no arrays of these structures,
   just pointers to them), etc. However, if you want to be *very* safe and
   support very drastic changes inside a major version, it needs to be opaque,
   so this could be the reason.

Is it either of these reasons, or is there another reason?

Making the ReadStream API non-opaque (that is, moving the definition to the
header file) would at least solve our problem (unless I am mistaken).
However, I am ignorant about long-term plans which might affect this, so
there might be a good reason to revert it for reasons I am not aware of.
-- 
Best wishes,
Mats Kindahl, Timescale

---

From: Thomas Munro <[email-masked]>
Date: Thu, 5 Sep 2024 21:12:07 +1200

On Thu, Sep 5, 2024 at 6:45 PM Mats Kindahl <[email-masked]> wrote:
> Forgive me for asking, but I am not entirely sure why the ReadStream struct is opaque. The usual reasons are:
>
> You want to provide an ABI to allow extensions to work with new major versions without re-compiling. Right now it is necessary to recompile extensions anyway, this does not seem to apply. (Because there are a lot of other changes that you need when switching versions because of the lack of a stable ABI for other parts of the code. However, it might be that the goal is to support it eventually, and then it would make sense to start making structs opaque.)
> You want to ensure that you can make modifications inside a major version without breaking ABIs and requiring a re-compile. In this case, you could still follow safe practice of adding new fields last, not relying on the size of the struct for anything (e.g., no arrays of these structures, just pointers to them), etc. However, if you want to be very safe and support very drastic changes inside a major version, it needs to be opaque, so this could be the reason.
>
> Making the ReadStream API non-opaque (that is, moving the definition to the header file) would at least solve our problem (unless I am mistaken). However, I am ignorant about long-term plans which might affect this, so there might be a good reason to revert it for reasons I am not aware of.

The second thing.  Also there are very active plans[1] to change the
internal design of ReadStream in 18, since the goal is to drive true
asynchronous I/O, and the idea of ReadStream was to create a simple
API to let many consumers start using it, so that we can drive
efficient modern system interfaces below that API, so having people
depending on how it works would not be great.

But let's talk about how that would actually look, for example if we
exposed the struct or you took a photocopy of it...  I think your idea
must be something like: if you could access struct ReadStream's
internals, you could replace stream->callback with an interceptor
callback, and if the BlockSampler had been given the fake N + M
relation size, the interceptor could overwrite
stream->ios[next_io_index].op.smgr and return x - N if the intercepted
callback returned x >= N.  (Small detail: need to check
stream->fast_path and use 0 instead or something like that, but maybe
we could change that.)  One minor problem that jumps out is that
read_stream.c could inappropriately merge blocks from the two
relations into one I/O.  Hmm, I guess you'd have to teach the
interceptor not to allow that: if switching between the two relation,
and if the block number would coincide with
stream->pending_read_blocknum + stream->pending_read_nblocks, it would
need to pick a new block instead (interfering with the block sampling
algorithm, but only very rarely).  Is this what you had in mind, or
something else?

(BTW I have a patch to teach read_stream.c about multi-smgr-relation
streams, by adding a different constructor with a different callback
that returns smgr, fork, block instead of just the block, but it
didn't make it into 17.)

[1] https://www.postgresql.org/message-id/flat/uvrtrknj4kdytuboidbhwclo4gxhswwcpgadptsjvjqcluzmah@brqs62irg4dt

---

From: Mats Kindahl <[email-masked]>
Date: Tue, 10 Sep 2024 19:07:18 +0200

On Thu, Sep 5, 2024 at 11:12 AM Thomas Munro <[email-masked]> wrote:

> > You want to provide an ABI to allow extensions to work with new major
> > You want to ensure that you can make modifications inside a major
> just pointers to them), etc. However, if you want to be very safe and
> > Making the ReadStream API non-opaque (that is, moving the definition to
> the header file) would at least solve our problem (unless I am mistaken).

That is understandable, since you usually do not want to have to re-compile
the extension for different minor versions. However, it would be a rare
case with extensions that are meddling with this, so might not turn out to
be a big problem in reality, as long as it is very clear to all involved
that this might change and that you make an effort to avoid binary
incompatibility by removing or changing types for fields.

> we could change that.)

Yes, this is what I had in mind, but I did not dig too deeply into the code.

> One minor problem that jumps out is that

Hmmm... I didn't look too closely at this. Since the block number comes
from the callback, I guess we could make sure to have a "padding" block
between the regions so that we "break" any suite of blocks, which I think
is what you mean with "teach the interceptor not to allow that", but I
would have to write a patch to make sure.

> that returns smgr, fork, block instead of just the block, but it

Without having looked at the patch, this sounds like the correct way to do
it.

>
> [1]
> https://www.postgresql.org/message-id/flat/uvrtrknj4kdytuboidbhwclo4gxhswwcpgadptsjvjqcluzmah@brqs62irg4dt
>

-- 
Best wishes,
Mats Kindahl, Timescale