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

Hi Michael,

On Wed, Oct 30, 2019 at 12:11 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> Looks fine. I have done some refinements as per the attached.

Thanks. This stood out to me:

+ * The result is a structure containing all the parsed option values in
+ * text-array format.

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.

> Running the regression tests of dummy_index_am has proved to be able
> to break the assertion you have added.

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:

int option_string_val_offset;
int option_string_null_offset;
} DummyIndexOptions;

but di_relopt_tab's last two entries are these:

add_string_reloption(di_relopt_kind, "option_string_val",
"String option for dummy_index_am with
non-NULL default",
"DefaultValue", &validate_string_option,
AccessExclusiveLock);
di_relopt_tab[4].optname = "option_string_val";
di_relopt_tab[4].opttype = RELOPT_TYPE_STRING;
di_relopt_tab[4].offset = offsetof(DummyIndexOptions,
option_string_val_offset);

/*
* String option for dummy_index_am with NULL default, and without
* description.
*/
add_string_reloption(di_relopt_kind, "option_string_null",
NULL, /* description */
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].offset = offsetof(DummyIndexOptions,
option_string_null_offset);

If I fix the above code like this:

@@ -113,7 +113,7 @@ create_reloptions_table(void)
"DefaultValue", &validate_string_option,
AccessExclusiveLock);
di_relopt_tab[4].optname = "option_string_val";
- di_relopt_tab[4].opttype = RELOPT_TYPE_STRING;
+ di_relopt_tab[4].opttype = RELOPT_TYPE_INT;
di_relopt_tab[4].offset = offsetof(DummyIndexOptions,
option_string_val_offset);

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

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

I haven't fully read parseRelOptions(), but I will trust you. :)

Thanks,
Amit