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
signature.asc
Description: PGP signature
