Re: ResourceOwner refactoring
От | Heikki Linnakangas |
---|---|
Тема | Re: ResourceOwner refactoring |
Дата | |
Msg-id | 3b6c2c31-0e2a-3656-c442-48bca31cc079@iki.fi обсуждение исходный текст |
Ответ на | RE: ResourceOwner refactoring ("kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>) |
Ответы |
RE: ResourceOwner refactoring
("kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>)
|
Список | 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
Вложения
В списке pgsql-hackers по дате отправления: