On Tue, Jul 25, 2023 at 02:35:31PM +0300, Aleksander Alekseev wrote: >> - Assert(cacheinfo[cacheId].reloid != 0); >> + Assert(cacheinfo[cacheId].reloid != InvalidOid); >> + Assert(cacheinfo[cacheId].indoid != InvalidOid); >> No objections about checking for the index OID given out to catch >> any failures at an early stage before doing an actual lookup? I guess >> that you've added an incorrect entry and noticed the problem only when >> triggering a syscache lookup for the new incorrect entry? >> + Assert(key[i] != InvalidAttrNumber); >> >> Yeah, same here. > > Not sure if I understand your question or suggestion. Thes proposed > patch adds Asserts() to InitCatalogCache() and InitCatCache() called > by it, before any actual lookups.
That was more a question. I was wondering if it was something you've noticed while working on a different patch because you somewhat assigned incorrect values in the syscache array, but it looks like you have noticed that while scanning the code. >> + Assert((cacheinfo[cacheId].nkeys > 0) && (cacheinfo[cacheId].nkeys >> <= 4)); >> >> Is this addition actually useful? > > I believe it is. One can mistakenly use 5+ keys in cacheinfo[] declaration, > e.g: Still it's hard to miss at compile time. I think that I would remove this one. > All in all I'm not claiming that these proposed new Asserts() make a > huge difference. I merely noticed that InitCatalogCache checks only > cacheinfo[cacheId].reloid. Checking the rest of the values would be > helpful for the developers and will not impact the performance of the > release builds. No counter-arguments on that. The checks for the index OID and the keys allow to catch failures in these structures at an earlier stage when initializing a backend. Agreed that it can be useful for developers. -- Michael
signature.asc
Description: PGP signature
