On Mon, Apr 22, 2024 at 06:46:27PM +0200, Matthias van de Meent wrote:
> On Mon, 22 Apr 2024 at 17:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
>>> 0002/0004 remove fields in ExecRowMark which were added for FDWs to
>>> use, but there are no FDWs which use this: I could only find two FDWs
>>> who implement RefetchForeignRow, one being BlackHoleFDW, and the other
>>> a no-op implementation in kafka_fdw [0]. We also don't seem to have
>>> any testing on this feature.
>>
>> I'm kind of down on removing either of these. ermExtra is explicitly
>> intended for extensions to use, and just because we haven't found any
>> users doesn't mean there aren't any, or might not be next week.
>
> That's a good point, and also why I wasn't 100% sure removing it was a
> good idea. I'm not quite sure why this would be used (rather than the
> internal state of the FDW, or no state at all), but haven't looked
> very deep into it either, so I'm quite fine with not channging that.
Custom nodes are one extra possibility? I'd leave ermActive and
ermExtra be.
>> I think it would be a good idea to push 0003 for v17, just so nobody
>> grows an unnecessary dependency on that field. 0001 and 0005 could
>> be left for v18, but on the other hand they're so trivial that it
>> could also be sensible to just push them to get them out of the way.
>
> Beta 1 scheduled to be released for quite some time, so I don't think
> there are any problems with fixing these kinds of minor issues in the
> provisional ABI for v17.
Tweaking the APIs should be OK until GA, as long as we agree that the
current interfaces can be improved.
0003 is new in v17, so let's apply it now. I don't see much a strong
argument in waiting for the removal of 0001 and 0005, either, to keep
the interfaces cleaner moving on. However, this is not a regression
and these have been around for years, so I'd suggest for v18 to open
before moving on with the removal.
I was wondering for a bit about how tss_htup could be abused in the
open, and the only references I can see come from forks of the
pre-2019 area, where this uses TidNext(). As a whole, ripping it out
does not stress me much.
--
Michael