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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Nikolay Shaplov <dhyan(at)nataraj(dot)su>, Dent John <denty(at)qqdd(dot)eu>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "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] Do not use StdRdOptions in Access Methods
Date: 2019-10-30 03:11:28
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Oct 28, 2019 at 05:16:54PM +0900, Amit Langote wrote:
> On Sat, Oct 26, 2019 at 11:45 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> On Fri, Oct 25, 2019 at 04:42:24PM +0900, Amit Langote wrote:
>> > Hmm, if we're inventing a new API to replace the old one, why not use
>> > that opportunity to be consistent with our general style, which
>> > predominantly seems to be either words_separated_by_underscore() or
>> > UpperCamelCase(). Thoughts?
>> Not wrong. Using small-case characters separated with underscores
>> would be more consistent with the rest perhaps? We use that for the
>> initialization of custom variables and for all the relkind-related
>> interfaces.
> OK, I went with build_reloptions(), which looks very similar to nearby
> exported functions.


>> + * Parses reloptions provided by the caller in text array format and
>> + * fills and returns a struct containing the parsed option values
>> The sentence structure is weird, perhaps:
>> This routine parses "reloptions" provided by the caller in text-array
>> format. The parsing is done with a table describing the allowed
>> options, defined by "relopt_elems" of length "num_relopt_elems". The
>> returned result is a structure containing all the parsed option
>> values.
> Thanks, I have expanded the header comment based on your text.

Looks fine. I have done some refinements as per the attached.

Running the regression tests of dummy_index_am has proved to be able
to break the assertion you have added. I don't have a good idea of
how to make that more simple and reliable, but there is one thing
outstanding here: the number of options parsed by parseRelOptions
should never be higher than num_relopt_elems. So let's at least be
safer about that.

Also if some options are parsed options will never be NULL, so there
is no need to check for it before pfree()-ing it, no?

Any comments from others? Alvaro perhaps?

Attachment Content-Type Size
build_reloptions-v4.patch text/x-diff 14.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2019-10-30 03:34:28 Re: Problem with synchronous replication
Previous Message Kyotaro Horiguchi 2019-10-30 02:18:56 Re: Make StringInfo available to frontend code.