Re: ResourceOwner refactoring

On 14/01/2021 12:15, kuroda(dot)hayato(at)fujitsu(dot)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