Hi, > hio.c: > > - saveFreeSpace = RelationGetTargetPageFreeSpace(relation, > - > HEAP_DEFAULT_FILLFACTOR); > + if (IsToastRelation(relation)) > + saveFreeSpace = ToastGetTargetPageFreeSpace(); > + else > + saveFreeSpace = HeapGetTargetPageFreeSpace(relation); > > This locution appears four times in the patch and that's the all where the > two macros appear. And it might not be good that RelationGetBufferForTuple > identifies a heap directly since I suppose that currently tost is a part of > heap. Thus it'd be better that HeapGetTargetPageFreeSpace handles the toast > case. > Similary, it doesn't looks good that RelationGetBufferForTuple consults > HeapGetTargretPageFreeSpace() directly. Perhaps it should be called via > TableGetTargetPageFreeSpace(). I'm not sure what is the right shape of the > relationship among a relation and a table and other kinds of relation. > extract_autovac_opts penetrates through the modularity boundary of > toast/heap in a similar way. I think so, too. And In current codes, RelationGetTargetPageFreeSpace users(ex. heap_page_prune_opt) don't have to think whatever target is toast or heap, but in your change, they need to use them properly.
You told us "big picture" about opclass around the beginning of this thread. In my understanding, the purpose of this refactoring is to make reloptions more flexible to add opclass. I understand this change may be needed for opclass, however I think that this is not the best way to refactor reloption. How about discussing more about this specification including opclass, and finding the best way to refactor reloption? I have not read the previous thread tightly, so you may have already started preparing. Regards, Aya Iwata
