On Tue, 2020-09-01 at 12:20 -0400, Alvaro Herrera wrote:
> Hmm, I think that if we're going to do this, we should do it for all
> AMs, not just table AMs, since surely index AMs also want extensible
> reloptions; and I think that makes the 'validate' mechanism dead code
> and so we should remove it.
Index AMs totally control the validation, so as they add options with
add_bool_reloption, they can also add to their custom parsing table.
I'm fine removing the "validate" parameter completely for the sake of
consistency.
> I think this means that you can have tables with mistyped options,
> and
> you'd not get an error message about them. Is that right? Is that
> what
> we want?
No, mistyped options (e.g. "fillfactory=50") will still cause an error,
because there are two layers of validation.
The problem case would be a situation like the following:
1. Someone loads an extension that creates a new reloption
"custom_reloption" of kind RELOPT_KIND_HEAP for their table access
method "my_tableam".
2. They then create a table and forget to specify "USING my_tableam",
but use the option "custom_reloption=123".
Ideally, that would throw an error because "custom_reloption" is only
valid for "my_tableam"; but with my patch, no error would be thrown
because the extension has already added the reloption. It would just
create a normal heap and "custom_reloption=123" would be ignored.
I went with the simple approach because fixing that problem seemed a
bit over-engineered. Here are some thoughts on what we could do:
* Consider StdRdOptions to be an extensible struct (where postgres just
looks at the StdRdOptions fields, but the extension can cast it to the
more specialized struct with extra fields). This is awkward, because
it's not a "normal" struct. Strings are represented as offsets that
point to memory past the end of the struct. We'd need an extra AM hook
that allocateReloptStruct can call into.
* We could refactor the validation logic so that extra options don't
count immediately as an error, but we instead save them in a different
array, and pass them to an AM-specific validation routine. But we have
no place to really put these options once they are parsed, because rel-
>rd_options is already holding the StdRdOptions struct, and there's no
separate place in the catalog for AM-specific reloptions. So the
extension would then need to re-parse them, and stash them in
rd_amcache or something.
* We could introduce a new AM routine that returns a custom
relopt_kind, created with add_reloption_kind(). Then, we could change
heap_reloptions to call default_reloptions like:
return default_reloptions(reloptions, validate,
RELOPT_KIND_HEAP|customReloptKind);
This still requires my patch, because we need to avoid the second
validation step. It also requires some extra code for TOAST tables,
because they can also be a custom table access method. The benefit over
my current patch is that extensions wouldn't be creating new options
for RELOPT_KIND_HEAP, they would create them only for their own custom
relopt kind, so if you try to use those options with a heap, you would
get an error.
Suggestions welcome.
Regards,
Jeff Davis
