Re: [PATCH] New [relation] option engine

On Wed, Feb 7, 2024 at 2:45 PM Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:
>
> В письме от четверг, 1 февраля 2024 г. 09:58:32 MSK пользователь Nikolay
> Shaplov написал:
>
> I' ve updated the patch again, since the output of the
> src/test/modules/test_oat_hooks test have changed.
> This test began to register namespace lookups, and since options internals
> have been changed much, number of lookups have been changed too. So I just
> fixed the expected file.
>
> > Hi!
> >
> > I've updated the patch, and it should work with modern master branch.
> >

you've changed the `typedef struct IndexAmRoutine`
then we also need to update doc/src/sgml/indexam.sgml.

some places you use "Spec Set", other places you use "spec set"
+typedef struct options_spec_set
+{
+ option_spec_basic **definitions;
+ int num; /* Number of spec_set items in use */
+ int num_allocated; /* Number of spec_set items allocated */
+ bool assert_on_realloc; /* If number of items of the spec_set were
+ * strictly set to certain value assert on
+ * adding more items */
+ bool is_local; /* If true specset is in local memory context */
+ Size struct_size; /* Size of a structure for options in binary
+ * representation */
+ postprocess_bytea_options_function postprocess_fun; /* This function is
+ * called after options
+ * were converted in
+ * Bytea representation.
+ * Can be used for extra
+ * validation etc. */
+ char *namspace; /* spec_set is used for options from this
+ * namespase */
+} options_spec_set;
here the comments, you used "spec_set".
maybe we can make it more consistent?

typedef enum option_value_status
{
OPTION_VALUE_STATUS_EMPTY, /* Option was just initialized */
OPTION_VALUE_STATUS_RAW, /* Option just came from syntax analyzer in
* has name, and raw (unparsed) value */
OPTION_VALUE_STATUS_PARSED, /* Option was parsed and has link to catalog
* entry and proper value */
OPTION_VALUE_STATUS_FOR_RESET, /* This option came from ALTER xxx RESET */
} option_value_status;

I am slightly confused.
say
`create table a(a1 int) with (foo=bar).`
to make table a created successfully:
we need to check if variable foo is valid or not, if not then error
out immediately,
we also need to check if the variable bar is within the expected bound.
overall I am not sure about the usage of option_value_status.
using some real example demo different option_value_status usage would be great.

+ /*
+ * If this option came from RESET statement we should throw error
+ * it it brings us name=value data, as syntax analyzer do not
+ * prevent it
+ */
+ if (def->arg != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("RESET must not include values for parameters")));

+ errmsg("RESET must not include values for parameters")));
the error message seems not so good?

you declared as:
options_spec_set *get_stdrd_relopt_spec_set(bool is_for_toast);
but you used as:
options_spec_set * get_stdrd_relopt_spec_set(bool is_heap)

maybe we refactor the usage as
`options_spec_set * get_stdrd_relopt_spec_set(bool is_for_toast)`

+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("Valid values are between \"%d\" and \"%d\".",
+ optint->min, optint->max)));

maybe
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("valid values are between %d and %d.",
+ optint->min, optint->max)));

+ if (validate && (option->values.real_val < optreal->min ||
+ option->values.real_val > optreal->max))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("Valid values are between \"%f\" and \"%f\".",
+ optreal->min, optreal->max)));

maybe
+ if (validate && (option->values.real_val < optreal->min ||
+ option->values.real_val > optreal->max))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("valid values are between %f and %f.",
+ optreal->min, optreal->max)));

I think the above two, change errdetail to errhint would be more appropriate.

I've attached v7, 0001 and 0002.
v7, 0001 is the rebased v6, v7, 0002 is miscellaneous minor cosmetic fix.