Re: [PATCH] Check more invariants during syscache initialization
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 signature.asc Description: PGP signature
Re: [PATCH] Check more invariants during syscache initialization
Hi, > On Jul 27, 2023, at 09:05, Michael Paquier 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
Re: [PATCH] Check more invariants during syscache initialization
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 signature.asc Description: PGP signature
Re: [PATCH] Check more invariants during syscache initialization
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 v3-0001-Check-more-invariants-during-syscache-initializat.patch Description: Binary data
Re: [PATCH] Check more invariants during syscache initialization
Aleksander Alekseev 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
Re: [PATCH] Check more invariants during syscache initialization
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
Re: [PATCH] Check more invariants during syscache initialization
HI, > On Jul 26, 2023, at 20:50, Aleksander Alekseev > 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 > 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
Re: [PATCH] Check more invariants during syscache initialization
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 Description: Binary data
Re: [PATCH] Check more invariants during syscache initialization
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
Re: [PATCH] Check more invariants during syscache initialization
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
Re: [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 signature.asc Description: PGP signature
[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 v1-0001-Check-more-invariants-during-syscache-initializat.patch Description: Binary data
