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

On Thu, Oct 31, 2019 at 4:49 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Thu, Oct 31, 2019 at 04:38:55PM +0900, Amit Langote wrote:
> > On Wed, Oct 30, 2019 at 12:11 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > This sentence sounds wrong, because the result structure doesn't
> > contain values in text-array format. Individual values in the struct
> > would be in their native format (C bool for RELOPT_TYPE_BOOL, options,
> > etc.).
> >
> > Maybe we don't need this sentence, because the first line already says
> > we return a struct.
>
> Let's remove it then.

Removed in the attached.

> > This breakage seems to have to do with the fact that the definition of
> > DummyIndexOptions struct and the entries of relopt_parse_elt table
> > don't agree?
> >
> > These are the last two members of DummyIndexOptions struct:
> >
> > @@ -126,7 +126,7 @@ create_reloptions_table(void)
> > NULL, &validate_string_option,
> > AccessExclusiveLock);
> > di_relopt_tab[5].optname = "option_string_null";
> > - di_relopt_tab[5].opttype = RELOPT_TYPE_STRING;
> > + di_relopt_tab[5].opttype = RELOPT_TYPE_INT;
> > di_relopt_tab[5].offset = offsetof(DummyIndexOptions,
> > option_string_null_offset);
> > }
> >
> > test passes.
> >
> > But maybe this Assert isn't all that robust, so I'm happy to take it out.
>
> This one should remain a string reloption, and what's stored in the
> structure is an offset to get the string. See for example around
> RelationHasCascadedCheckOption before it got switched to an enum in
> 773df88 regarding the way to get the value.

I see. I didn't know that about STRING options when writing that
Assert, so it was indeed broken to begin with.

v5 attached.

Thanks,
Amit