From: | Dent John <denty(at)QQdd(dot)eu> |
---|---|
To: | Nikolay Shaplov <dhyan(at)nataraj(dot)su> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, "Iwata, Aya" <iwata(dot)aya(at)jp(dot)fujitsu(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> |
Subject: | Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead |
Date: | 2019-07-20 08:21:42 |
Message-ID: | F1BDB89D-A7EA-4E26-B6E7-CA305BD7C3BC@QQdd.eu |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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>
From | Date | Subject | |
---|---|---|---|
Next Message | Dilip Kumar | 2019-07-20 08:25:45 | Re: POC: Cleaning up orphaned files using undo logs |
Previous Message | Amit Kapila | 2019-07-20 07:10:18 | Re: POC: Cleaning up orphaned files using undo logs |