Re: ResourceOwner refactoring - Mailing list pgsql-hackers

On 14/01/2021 12:15, kuroda.hayato@fujitsu.com wrote:
> I put some comments.

Thanks for the review!

> Throughout, some components don’t have helper functions.
> (e.g. catcache has ResOwnerReleaseCatCache, but tupdesc doesn't.)
> I think it should be unified.

Hmm. ResOwnerReleaseTupleDesc() does exist, those functions are needed 
for the callbacks. I think you meant the wrappers around 
ResourceOwnerRemember and ResourceOwnerForget, like 
ResourceOwnerRememberCatCacheRef(). I admit those are not fully 
consistent: I didn't bother with the wrapper functions when there is 
only one caller.

> [catcache.c]
>> +/* support for catcache refcount management */
>> +static inline void
>> +ResourceOwnerEnlargeCatCacheRefs(ResourceOwner owner)
>> +{
>> +       ResourceOwnerEnlarge(owner);
>> +}
> 
> This function is not needed.

Removed.

> [syscache.c]
>> -static CatCache *SysCache[SysCacheSize];
>> + CatCache *SysCache[SysCacheSize];
> 
> Is it right? Compilation is done even if this variable is static...

Fixed. (It was a leftover from when I was playing with Kyotaro's 
"catcachebench" tool from another thread).

> [fd.c, dsm.c]
> In these files helper functions are implemented as the define directive.
> Could you explain the reason? For the performance?

No particular reason. I turned them all into macros for consistency.

>> Previously, this was OK, because resources AAA and BBB were kept in
>> separate arrays. But after this patch, it's not guaranteed that the
>> ResourceOwnerRememberBBB() will find an empty slot.
>>
>> I don't think we do that currently, but to be sure, I'm planning to grep
>> ResourceOwnerRemember and look at each call carefullly. And perhaps we
>> can add an assertion for this, although I'm not sure where.
> 
> Indeed, but I think this line works well, isn't it?
> 
>>     Assert(owner->narr < RESOWNER_ARRAY_SIZE);

That catches cases where you actually overrun the array, but it doesn't 
catch unsafe patterns when there happens to be enough space left in the 
array. For example, if you have code like this:

/* Make sure there's room for one more entry, but remember *two* things */
ResourceOwnerEnlarge();
ResourceOwnerRemember(foo);
ResourceOwnerRemember(bar);

That is not safe, but it would only fail the assertion if the first 
ResourceOwnerRemember() call happens to consume the last remaining slot 
in the array.


I checked all the callers of ResourceOwnerEnlarge() to see if they're 
safe. A couple of places seemed a bit suspicious. I fixed them by moving 
the ResourceOwnerEnlarge() calls closer to the ResourceOwnerRemember() 
calls, so that it's now easier to see that they are correct. See first 
attached patch.

The second patch is an updated version of the main patch, fixing all the 
little things you and Michael pointed out since the last patch version.

I've been working on performance testing too. I'll post more numbers 
later, but preliminary result from some micro-benchmarking suggests that 
the new code is somewhat slower, except in the common case that the 
object to remember and forget fits in the array. When running the 
regression test suite, about 96% of ResourceOwnerForget() calls fit in 
the array. I think that's acceptable.

- Heikki