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

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
Views: Raw Message | Whole Thread | Download mbox | Resend email
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.


> 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>

In response to

Browse pgsql-hackers by date

  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