Re: New Object Access Type hooks

On 3/18/22 11:15, Mark Dilger wrote:
>
>> On Mar 18, 2022, at 7:16 AM, Joshua Brindle <joshua(dot)brindle(at)crunchydata(dot)com> wrote:
>>
>> This is great, thank you for doing this. I didn't even realize the OAT
>> hooks had no regression tests.
>>
>> It looks good to me, I reviewed both and tested the module. I wonder
>> if the slight abuse of subid is warranted with brand new hooks going
>> in but not enough to object, I just hope this doesn't rise to the too
>> large to merge this late level.

The core code is extracted from a current CF patch, so I think in
principle it's OK.

I haven't looked at it in detail, but regarding the test code I'm not
sure why there's a .control file, since this isn't a loadable extension,
not why there's a test_oat_hooks.h file.

> The majority of the patch is regression testing code, stuff which doesn't get installed. It's even marked as NO_INSTALLCHECK, so it won't get installed even as part of "make installcheck". That seems safe enough to me.
>
> Not including tests of OAT seems worse, as it leaves us open to breaking the behavior without realizing we've done so. A refactoring of the core code might cause hooks to be called in a different order, something which isn't necessarily wrong, but should not be done unknowingly.
>

Yes, and in any case we've added test code after feature freeze in the past.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com