Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

Hi Nikolay,

Thanks for the revised patch. It applies now no problem, and seems to work fine.

For me, I still find the relopts area quite odd. I wonder if your patch doesn’t go far enough?

For example, take log_autovacuum_min_duration. It’s described intRelOpts, which implicitly defines its type (an int), and explicitly sets its min/max, default, and lock level, as well as it’s kind (HEAP | TOAST). But then it’s described again in the relopt_parse_elt[] in toast_reloptions() and heap_reloptions(), which implicitly defines it to apply to HEAP | TOAST, and fact of it being an int. (It’s, of course, same for all reloptions.)

The relopt_parse_elt[] is hugely entirely duplicative. I wonder if it is not best simply to consolidate — either push all info into the static {bool,int,real,string}RelOpts[] structures, or push all into the relopt_parse_elt[].

I notice the thread earlier talks of some of the APIs being public interfaces, which may be used by EXTENSIONs. I’m not sure I buy that in its fullest sense. For sure, an EXTENSION may invoke the APIs. But no EXTENSION can define new/alter existing options, because the {bool,int,real,string}RelOpts[] are not currently runtime-extendable. I guess we probably should preserve the API’s functionality, and allow fillRelOptions(), if provided with a tab, for it to restrict filling to only those supplied in the tab. But in general core code, my opinion is that fillRelOptions() could be provided with a NULL tab, and for it to scavenge all needed information from the static {bool,int,real,string}RelOpts[] structures.

That links to what I initially found most confusing: AUTOVACUUM_RELOPTIONS. I understand it’s there because there are a bunch of shared reloptions. Pre-patch, default_reloptions() meant there was no need for the macro, but your patch drops default_reloptions(). default_reloptions() is horrible, but I feel the macro approach is worse. :-)

Sorry for the delay providing the feedback. It took me a few sittings to grok what was going on, and to work out what I though about it.

denty.

> On 12 Jul 2019, at 15:13, Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:
>
> В письме от четверг, 4 июля 2019 г. 19:44:42 MSK пользователь Dent John
> написал:
>> Hi Nikolay,
>>
>> I have had a crack at re-basing the current patch against 12b2, but I didn’t
>> trivially succeed.
>>
>> It’s probably more my bodged fixing of the rejections than a fundamental
>> problem. But I now get compile fails in — and seems like only — vacuum.c.
>
> I am really sorry for such big delay. Two new relation options were added, it
> needed careful checking while importing them back to the patch.
> I did not find proper timeslot for doing this quick enough.
> Hope I am not terribly late.
> Here goes an updated version.
>
>
>
> <get-rid-of-StrRdOptions_5.diff>