Re: [PATCH] Do not use StdRdOptions in Access Methods

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 <dhyan(at)nataraj(dot)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>