Horton

Thread

Commits

  1. Add sanity asserts for index OID and attnums during cache init

  1. [PATCH] Check more invariants during syscache initialization

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-24T18:58:15Z

    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

  2. Re: [PATCH] Check more invariants during syscache initialization

    Michael Paquier <michael@paquier.xyz> — 2023-07-24T23:02:36Z

    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

  3. Re: [PATCH] Check more invariants during syscache initialization

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-25T11:35:31Z

    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

  4. Re: [PATCH] Check more invariants during syscache initialization

    Michael Paquier <michael@paquier.xyz> — 2023-07-25T23:45:10Z

    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

  5. Re: [PATCH] Check more invariants during syscache initialization

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-26T12:50:53Z

    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

  6. Re: [PATCH] Check more invariants during syscache initialization

    Zhang Mingli <zmlpostgres@gmail.com> — 2023-07-26T13:29:53Z

    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

  7. Re: [PATCH] Check more invariants during syscache initialization

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-26T13:59:31Z

    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

  8. Re: [PATCH] Check more invariants during syscache initialization

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-07-26T14:28:55Z

    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

  9. Re: [PATCH] Check more invariants during syscache initialization

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-26T16:01:11Z

    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

  10. Re: [PATCH] Check more invariants during syscache initialization

    Michael Paquier <michael@paquier.xyz> — 2023-07-27T01:05:46Z

    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

  11. Re: [PATCH] Check more invariants during syscache initialization

    Zhang Mingli <zmlpostgres@gmail.com> — 2023-07-27T01:48:10Z

    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

  12. Re: [PATCH] Check more invariants during syscache initialization

    Michael Paquier <michael@paquier.xyz> — 2023-07-27T01:57:20Z

    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