Re: [PATCH] Check more invariants during syscache initialization - Mailing list pgsql-hackers

Hi Michael,

> -       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.

> +       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:

```
diff --git a/src/backend/utils/cache/syscache.c
b/src/backend/utils/cache/syscache.c
index 4883f36a67..c7a8a5b8bb 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -159,6 +159,7 @@ static const struct cachedesc cacheinfo[] = {
                KEY(Anum_pg_amop_amopfamily,
                        Anum_pg_amop_amoplefttype,
                        Anum_pg_amop_amoprighttype,
+                       Anum_pg_amop_amoplefttype,
                        Anum_pg_amop_amopstrategy),
                64
        },
```

What happens in this case, at least with GCC - the warning is shown
but the code compiles fine:

```
1032/1882] Compiling C object
src/backend/postgres_lib.a.p/utils_cache_syscache.c.o
src/include/catalog/pg_amop_d.h:30:35: warning: excess elements in
array initializer
   30 | #define Anum_pg_amop_amopstrategy 5
      |                                   ^
../src/backend/utils/cache/syscache.c:127:48: note: in definition of macro ‘KEY’
  127 | #define KEY(...) VA_ARGS_NARGS(__VA_ARGS__), { __VA_ARGS__ }
      |                                                ^~~~~~~~~~~
../src/backend/utils/cache/syscache.c:163:25: note: in expansion of
macro ‘Anum_pg_amop_amopstrategy’
  163 |                         Anum_pg_amop_amopstrategy),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
src/include/catalog/pg_amop_d.h:30:35: note: (near initialization for
‘cacheinfo[4].key’)
   30 | #define Anum_pg_amop_amopstrategy 5
      |                                   ^
../src/backend/utils/cache/syscache.c:127:48: note: in definition of macro ‘KEY’
  127 | #define KEY(...) VA_ARGS_NARGS(__VA_ARGS__), { __VA_ARGS__ }
      |                                                ^~~~~~~~~~~
../src/backend/utils/cache/syscache.c:163:25: note: in expansion of
macro ‘Anum_pg_amop_amopstrategy’
  163 |                         Anum_pg_amop_amopstrategy),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
```

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.

--
Best regards,
Aleksander Alekseev