Again, thanks a lot for the feedback.
On 2020-12-02 12:03 p.m., Fabien COELHO wrote:
>
> Hello David,
>
> Some feedback about v4.
>
>>> It looks that the option is *silently* ignored when creating a
>>> partitionned table, which currently results in an error, and not
>>> passed to partitions, which would accept them. This is pretty weird.
>> The input check is added with an error message when both partitions
>> and table access method are used.
>
> Hmmm. If you take the resetting the default, I do not think that you
> should have to test anything? AFAICT the access method is valid on
> partitions, although not on the partitioned table declaration. So I'd
> say that you could remove the check.
Yes, it makes sense to remove the *blocking* check, and actually the
table access method interface does work with partitioned table. I tested
this/v5 by duplicating the heap access method with a different name. For
this reason, I removed the condition check as well when applying the
table access method.
>
>>> They should also trigger failures, eg
>>> "--table-access-method=no-such-table-am", to be added to the @errors
>>> list.
>> No sure how to address this properly, since the table access method
>> potentially can be *any* name.
>
> I think it is pretty unlikely that someone would chose the name
> "no-such-table-am" when developing a new storage engine for Postgres
> inside Postgres, so that it would make this internal test fail.
>
> There are numerous such names used in tests, eg "no-such-database",
> "no-such-command".
>
> So I'd suggest to add such a test to check for the expected failure.
The test case "no-such-table-am" has been added to the errors list, and
the regression test is ok.
>
>>> I do not understand why you duplicated all possible option entry.
>>>
>>> Test with just table access method looks redundant if the feature is
>>> make to work orthonogonally to partitions, which should be the case.
>> Only one positive test case added using *heap* as the table access
>> method to verify the initialization.
>
> Yep, only "heap" if currently available for tables.
>
Thanks,
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca