Re: Use streaming read API in ANALYZE

On Thu, Sep 5, 2024 at 1:34 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:

> On Thu, Sep 5, 2024 at 3:36 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Wed, Sep 4, 2024 at 6:38 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> 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?
>
> 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.
>

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