Re: Table AM Interface Enhancements

On Tue, 02 Apr 2024 at 07:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Mon, Apr 1, 2024 at 7:36 PM Tom Lane <tgl@sss.pgh.pa.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