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