Horton

Re: Table AM Interface Enhancements

Japin Li <japinli@hotmail.com>

View thread View on postgr.es
From: Japin Li <japinli@hotmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andrei Lepikhov <a.lepikhov@postgrespro.ru>, Pavel Borisov <pashkin.elfe@gmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-04-02T00:57:47Z
Lists: pgsql-hackers 
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

Commits

  1. Remove extra comment at TableAmRoutine.scan_analyze_next_block

  2. revert: Generalize relation analyze in table AM interface

  3. Revert: Allow table AM to store complex data structures in rd_amcache

  4. Revert: Allow table AM tuple_insert() method to return the different slot

  5. Revert: Allow locking updated tuples in tuple_update() and tuple_delete()

  6. Revert: Let table AM insertion methods control index insertion

  7. Revert: Custom reloptions for table AM

  8. Provide a way block-level table AMs could re-use acquire_sample_rows()

  9. Custom reloptions for table AM

  10. Use streaming I/O in ANALYZE.

  11. Revert "Custom reloptions for table AM"

  12. Let table AM insertion methods control index insertion

  13. Generalize relation analyze in table AM interface

  14. Improve error message for tts_(virtual|minimal)_is_current_xact_tuple

  15. Add comments on some MinimalTupleSlots methods usage

  16. Add TupleTableSlotOps.is_current_xact_tuple() method

  17. Allow table AM tuple_insert() method to return the different slot

  18. Allow table AM to store complex data structures in rd_amcache