Обсуждение: [PATCH] Check more invariants during syscache initialization
Hi, Currently InitCatalogCache() has only one Assert() for cacheinfo[] that checks .reloid. The proposed patch adds sanity checks for the rest of the fields. -- Best regards, Aleksander Alekseev
Вложения
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