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

Hi Michael,

On Wed, Oct 30, 2019 at 12:11 PM Michael Paquier <michael@paquier.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