[PATCH] Check more invariants during syscache initialization

On Mon, Jul 24, 2023 at 09:58:15PM +0300, Aleksander Alekseev wrote:

Currently InitCatalogCache() has only one Assert() for cacheinfo[]
that checks .reloid. The proposed patch adds sanity checks for the
rest of the fields.

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

+ Assert((cacheinfo[cacheId].nkeys > 0) && (cacheinfo[cacheId].nkeys <= 4));

Is this addition actually useful? The maximum number of lookup keys
is enforced at API level with the SearchSysCache() family. Or perhaps
you've been able to break this layer in a way I cannot imagine and
were looking at a quick way to spot the inconsistency?
--
Michael

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

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

Hi Michael,

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.

Oh, got it. That's correct.

Still it's hard to miss at compile time. I think that I would remove
this one.

Fair enough. Here is the updated patch.

--
Best regards,
Aleksander Alekseev

HI,

On Jul 26, 2023, at 20:50, Aleksander Alekseev <aleksander@timescale.com> wrote:

Hi Michael,

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.

Oh, got it. That's correct.

Still it's hard to miss at compile time. I think that I would remove
this one.

Fair enough. Here is the updated patch.

--
Best regards,
Aleksander Alekseev
<v2-0001-Check-more-invariants-during-syscache-initializat.patch>

LGTM.

```
-		Assert(cacheinfo[cacheId].reloid != 0);
+		Assert(cacheinfo[cacheId].reloid != InvalidOid);
```

That remind me to have a look other codes, and a grep search `oid != 0` show there are several files using old != 0.

```
.//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0)
.//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0)
.//src/bin/pg_dump/pg_backup_tar.c: if (oid != 0)
.//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0)
.//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0)
```
That is another story…I would like provide a patch if it worths.

Zhang Mingli
https://www.hashdata.xyz

Hi Zhang,

That remind me to have a look other codes, and a grep search `oid != 0` show there are several files using old != 0.

```
.//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0)
.//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0)
.//src/bin/pg_dump/pg_backup_tar.c: if (oid != 0)
.//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0)
.//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0)
```
That is another story…I would like provide a patch if it worths.

Good catch. Please do so.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:

Hi Zhang,

That remind me to have a look other codes, and a grep search `oid != 0` show there are several files using old != 0.

```
.//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0)
.//src/bin/pg_resetwal/pg_resetwal.c: if (set_oid != 0)
.//src/bin/pg_dump/pg_backup_tar.c: if (oid != 0)
.//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0)
.//src/bin/pg_dump/pg_backup_custom.c: while (oid != 0)
```
That is another story…I would like provide a patch if it worths.

Good catch. Please do so.

Shouldn't these be calling `OidIsValid(…)`, not comparing directly to
`InvalidOid`?

~/src/postgresql (master $)$ git grep '[Oo]id [!=]= 0' | wc -l
18
~/src/postgresql (master $)$ git grep '[!=]= InvalidOid' | wc -l
296
~/src/postgresql (master $)$ git grep 'OidIsValid' |
wc -l
1462

- ilmari

Hi,

Shouldn't these be calling `OidIsValid(…)`, not comparing directly to
`InvalidOid`?

They should, thanks. Here is the updated patch. I made sure there are
no others != InvalidOid checks in syscache.c and catcache.c which are
affected by my patch. I didn't change any other files since Zhang
wants to propose the corresponding patch.

--
Best regards,
Aleksander Alekseev

On Wed, Jul 26, 2023 at 03:28:55PM +0100, Dagfinn Ilmari Mannsåker wrote:

Shouldn't these be calling `OidIsValid(…)`, not comparing directly to
`InvalidOid`?

I think that they should use OidIsValid() on correctness ground, and
that's the style I prefer. Now, I don't think that these are worth
changing now except if some of the surrounding code is changed for a
different reason. One reason is that this increases the odds of
conflicts when backpatching on a stable branch.
--
Michael

Hi,

On Jul 27, 2023, at 09:05, Michael Paquier <michael@paquier.xyz> wrote:

One reason is that this increases the odds of
conflicts when backpatching on a stable branch.

Agree. We could suggest to use OidIsValid() for new patches during review.

Zhang Mingli
https://www.hashdata.xyz

On Wed, Jul 26, 2023 at 07:01:11PM +0300, Aleksander Alekseev wrote:

Hi,

Shouldn't these be calling `OidIsValid(…)`, not comparing directly to
`InvalidOid`?

They should, thanks. Here is the updated patch. I made sure there are
no others != InvalidOid checks in syscache.c and catcache.c which are
affected by my patch. I didn't change any other files since Zhang
wants to propose the corresponding patch.

While arguing about OidIsValid() for the relation OID part, I found a
bit surprising that you did not consider AttributeNumberIsValid() for
the new check on the keys. I've switched the second check to that,
and applied v3.
--
Michael