Re: Table AM Interface Enhancements

On Tue, 02 Apr 2024 at 07:59, Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> On Mon, Apr 1, 2024 at 7:36 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Coverity complained about what you did in RelationParseRelOptions
>> in c95c25f9a:
>>
>> *** CID 1595992: Null pointer dereferences (FORWARD_NULL)
>> /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/cache/relcache.c: 499 in RelationParseRelOptions()
>> 493
>> 494 /*
>> 495 * Fetch reloptions from tuple; have to use a hardwired descriptor because
>> 496 * we might not have any other for pg_class yet (consider executing this
>> 497 * code for pg_class itself)
>> 498 */
>> >>> CID 1595992: Null pointer dereferences (FORWARD_NULL)
>> >>> Passing null pointer "tableam" to "extractRelOptions", which dereferences it.
>> 499 options = extractRelOptions(tuple, GetPgClassDescriptor(),
>> 500 tableam, amoptsfn);
>> 501
>>
>> I see that extractRelOptions only uses the tableam argument for some
>> relkinds, and RelationParseRelOptions does set it up for those
>> relkinds --- but Coverity's complaint isn't without merit, because
>> those two switch statements are looking at *different copies of the
>> relkind*, which in theory could be different. This all seems quite
>> messy and poorly factored. Can't we do better? Why do we need to
>> involve two copies of allegedly the same pg_class tuple, anyhow?
>
> I wasn't registered at Coverity yet. Now I've registered and am
> waiting for approval to access the PostgreSQL analysis data.
>
> I wonder why Coverity complains about tableam, but not amoptsfn.
> Their usage patterns are very similar.
>
> It appears that relation->rd_rel isn't the full copy of pg_class tuple
> (see AllocateRelationDesc). RelationParseRelOptions() is going to
> update relation->rd_options, and thus needs a full pg_class tuple to
> fetch options out of it. However, it is really unnecessary to access
> both tuples at the same time. We can use a full tuple, not
> relation->rd_rel, in both cases. See the attached patch.
>
> ------
> Regards,
> Alexander Korotkov

+ Form_pg_class classForm = (Form_pg_class) GETSTRUCT(tuple);
+;

There is an additional semicolon in the code.

--
Regards,
Japin Li