Horton

Thread

Commits

  1. Add missing ObjectIdGetDatum() in syscache lookup calls for Oids

  1. ObjectIdGetDatum() missing from SearchSysCache*() callers

    Michael Paquier <michael@paquier.xyz> — 2023-07-17T11:10:34Z

    Hi all,

While scanning the code, I have noticed that a couple of code paths
that do syscache lookups are passing down directly Oids rather than
Datums.  I think that we'd better be consistent here, even if there is
no actual bug.

I have noticed 11 callers of SearchSysCache*() that pass down
an Oid instead of a Datum.

Thoughts or comments?
--
Michael

  2. Re: ObjectIdGetDatum() missing from SearchSysCache*() callers

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-17T12:36:45Z

    Hi,

> I have noticed 11 callers of SearchSysCache*() that pass down
> an Oid instead of a Datum.

Good catch.

> I think that we'd better be consistent here, even if there is
> no actual bug.
>

+1

-- 
Best regards,
Aleksander Alekseev

  3. Re: ObjectIdGetDatum() missing from SearchSysCache*() callers

    Zhang Mingli <zmlpostgres@gmail.com> — 2023-07-17T13:09:01Z

    Hi

Regards,
Zhang Mingli
On Jul 17, 2023 at 19:10 +0800, Michael Paquier <michael@paquier.xyz>, wrote:
> Hi all,
>
> While scanning the code, I have noticed that a couple of code paths
> that do syscache lookups are passing down directly Oids rather than
> Datums. I think that we'd better be consistent here, even if there is
> no actual bug.
>
> I have noticed 11 callers of SearchSysCache*() that pass down
> an Oid instead of a Datum.
>
> Thoughts or comments?
> --
> Michael
LGTM, and there are two functions missed, in sequence_options

 pgstuple = SearchSysCache1(SEQRELID, relid);

Shall we fix that too?

  4. Re: ObjectIdGetDatum() missing from SearchSysCache*() callers

    Zhang Mingli <zmlpostgres@gmail.com> — 2023-07-17T13:11:22Z

    Hi,

Regards,
Zhang Mingli
On Jul 17, 2023 at 21:09 +0800, Zhang Mingli <zmlpostgres@gmail.com>, wrote:
> sequence_options
And inside pg_sequence_parameters:
	pgstuple = SearchSysCache1(SEQRELID, relid);

  5. Re: ObjectIdGetDatum() missing from SearchSysCache*() callers

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-17T14:33:42Z

    Hi,

> And inside pg_sequence_parameters:
> pgstuple = SearchSysCache1(SEQRELID, relid);

Found another one in partcache.c:

```
    /* Get pg_class.relpartbound */
    tuple = SearchSysCache1(RELOID, RelationGetRelid(rel));
```

I can't be 100% sure but it looks like that's all of them. PFA the
updated patch v2.

-- 
Best regards,
Aleksander Alekseev

  6. Re: ObjectIdGetDatum() missing from SearchSysCache*() callers

    Aleksander Alekseev <aleksander@timescale.com> — 2023-07-17T14:38:41Z

    Hi,

> > And inside pg_sequence_parameters:
> > pgstuple = SearchSysCache1(SEQRELID, relid);
>
> Found another one in partcache.c:
>
> ```
>     /* Get pg_class.relpartbound */
>     tuple = SearchSysCache1(RELOID, RelationGetRelid(rel));
> ```
>
> I can't be 100% sure but it looks like that's all of them. PFA the
> updated patch v2.

Added a CF entry, just in case:
https://commitfest.postgresql.org/44/4448/


-- 
Best regards,
Aleksander Alekseev

  7. Re: ObjectIdGetDatum() missing from SearchSysCache*() callers

    Michael Paquier <michael@paquier.xyz> — 2023-07-17T22:27:02Z

    On Mon, Jul 17, 2023 at 05:33:42PM +0300, Aleksander Alekseev wrote:
> I can't be 100% sure but it looks like that's all of them. PFA the
> updated patch v2.

Thanks.  Yes, this stuff is easy to miss.  I was just grepping for a
few patterns and missed these two.
--
Michael

  8. Re: ObjectIdGetDatum() missing from SearchSysCache*() callers

    Michael Paquier <michael@paquier.xyz> — 2023-07-20T06:28:19Z

    On Tue, Jul 18, 2023 at 07:27:02AM +0900, Michael Paquier wrote:
> On Mon, Jul 17, 2023 at 05:33:42PM +0300, Aleksander Alekseev wrote:
> > I can't be 100% sure but it looks like that's all of them. PFA the
> > updated patch v2.
> 
> Thanks.  Yes, this stuff is easy to miss.  I was just grepping for a
> few patterns and missed these two.

Spotted a few more of these things after a second lookup.

One for subscriptions:
src/backend/commands/alter.c:
if (SearchSysCacheExists2(SUBSCRIPTIONNAME, MyDatabaseId,

And two for transforms:
src/backend/utils/cache/lsyscache.c:
tup = SearchSysCache2(TRFTYPELANG, typid, langid);
src/backend/utils/cache/lsyscache.c:
tup = SearchSysCache2(TRFTYPELANG, typid, langid);

And applied the whole.  Thanks for looking and spot more of these
inconsistencies!
--
Michael