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.
> Similarly, ermActive seems like it's at least potentially useful:
> is there another way for onlookers to discover that state?
The ermActive field is always true when RefetchForeignRow is called
(in ExecLockRows(), in nodeLockRows.c), and we don't seem to care
about the value of the field afterwards. Additionally, we always set
erm->curCtid to a valid value when ermActive is also first set in that
code path.
In all, it feels like a duplicative field with no real uses inside
PostgreSQL itself. If an extension (FDW) needs it, it should probably
use ermExtra instead, as ermActive seemingly doesn't carry any
meaningful value into the FDW call.
> 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.
Kind regards,
Matthias van de Meent