Hi Nikolay, I like the new approach. And I agree with the ambition — to split out the representation from StdRdOptions.
However, with that change, in the AM’s *options() function, it looks as if you could simply add new fields to the relopt_parse_elt list. That’s still not true, because parseRelOptions() will fail to find a matching entry, causing numoptions to be left zero, and an early exit made. (At least, that’s if I correctly understand how things work.) I think that is fine as an interim limitation, because your change has not yet fully broken the connection to the boolRelOpts, intRelOpts, realRelOpts and stringRelOpts strutures. But perhaps a comment would help to make it clear. Perhaps add something like this above the tab[]: "When adding or changing a relopt in the relopt_parse_elt tab[], ensure the corresponding change is made to boolRelOpts, intRelOpts, realRelOpts or stringRelOpts." denty. > On 6 Oct 2019, at 14:45, Nikolay Shaplov <dh...@nataraj.su> wrote: > > Hi! I am starting a new thread as commitfest wants new thread for new patch. > > This new thread is a follow up to an https://www.postgresql.org/message-id/ > 2620882.s52SJui4ql%40x200m thread, where I've been trying to get rid of > StdRdOpions structure, and now I've splitted the patch into smaller parts. > > Read the quote below, to get what this patch is about > >> I've been thinking about this patch and came to a conclusion that it >> would be better to split it to even smaller parts, so they can be >> easily reviewed, one by one. May be even leaving most complex >> Heap/Toast part for later. >> >> This is first part of this patch. Here we give each Access Method it's >> own option binary representation instead of StdRdOptions. >> >> I think this change is obvious. Because, first, Access Methods do not >> use most of the values defined in StdRdOptions. >> >> Second this patch make Options structure code unified for all core >> Access Methods. Before some AM used StdRdOptions, some AM used it's own >> structure, now all AM uses own structures and code is written in the >> same style, so it would be more easy to update it in future. >> >> John Dent, would you join reviewing this part of the patch? I hope it >> will be more easy now... > > And now I have a newer version of the patch, as I forgot to remove > vacuum_cleanup_index_scale_factor from StdRdOptions as it was used only in > Btree index and now do not used at all. New version fixes it. > > -- > Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da > Body-oriented Therapist: https://vk.com/nataraj_rebalancing > (Russian)<do-not-use-StdRdOptions-in-AM_2.diff>
