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.brindle@crunchydata.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
necessarilywrong, 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