On Thu, Aug 01, 2019 at 09:39:53PM +1200, Thomas Munro wrote:
> Looks like some good actionable feedback. I've moved this patch to
> September, and set it to "Waiting on Author".
The patch is in this state for two months now, so I have switched it
to "returned with feedback". The latest patch does not apply, and it
would require an update for the new test module dummy_index_am.
As I have touched recently in the area of the code, I got a look at it
and the core of the idea is that it would be cleaner to move the
control of reloptions from reloptions.c to each AM, as well as have an
extra later for toast. This has additional costs in multiple ways:
- The parsing and filling of reloptions gets more duplicated, though
this could be solved with a wrapper routine (something HEAD already
does when filling in rd_options).
- Logic which was out of toast previously is not anymore.
Some of these have been mentioned by Tomas upthread, and I share
similar points. I also doubt that the removal of
RelationGetTargetPageFreeSpace() is a good thing, and this complicates
more the checks around options and the handling of rd_options which
may become optionally NULL, making a bit more brittle the whole
structure. We may be able to get useful pieces for it, though it is
not clear what would be the benefits. It may help as well to group
that within the thread dedicated to the rework of the reloption APIs
as a preliminary, refactoring step.
--
Michael