Re: Table AM Interface Enhancements

On Thu, 28 Mar 2024 at 21:26, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> Hi Pavel!
>
> Revised patchset is attached.
>
> On Thu, Mar 28, 2024 at 3:12 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>> The other extensibility that seems quite clear and uncontroversial to me is 0006.
>>
>> It simply shifts the decision on whether tuple inserts should invoke inserts to the related indices to the table am
level.It doesn't change the current heap insert behavior so it's safe for the existing heap access method. But new
tableaccess methods could redefine this (only for tables created with these am's) and make index inserts independently
ofExecInsertIndexTuples inside their own implementations of tuple_insert/tuple_multi_insert methods. 
>>
>> I'd propose changing the comment:
>>
>> 1405  * This function sets `*insert_indexes` to true if expects caller to return
>> 1406  * the relevant index tuples.  If `*insert_indexes` is set to false, then
>> 1407  * this function cares about indexes itself.
>>
>> in the following way
>>
>> Tableam implementation of tuple_insert should set `*insert_indexes` to true
>> if it expects the caller to insert the relevant index tuples (as in heap
>>  implementation). It should set `*insert_indexes` to false if it cares
>> about index inserts itself and doesn't want the caller to do index inserts.
>
> Changed as you proposed.
>
>> Maybe, a commit message is also better to reformulate to describe better who should do what.
>
> Done.
>
> Also, I removed extra includes in 0001 as you proposed and edited the
> commit message in 0002.
>
>> I think, with rebase and correction in the comments/commit message patch 0006 is ready to be committed.
>
> I'm going to push 0001, 0002 and 0006 if no objections.

Thanks for updating the patches.  Here are some minor sugesstion.

0003

+static inline TupleTableSlot *
+heapam_tuple_insert_with_arbiter(ResultRelInfo *resultRelInfo,

I'm not entirely certain whether the "inline" keyword has any effect.

0004

+static bytea *
+heapam_indexoptions(amoptions_function amoptions, char relkind,
+                                       Datum reloptions, bool validate)
+{
+       return index_reloptions(amoptions, reloptions, validate);
+}

Could you please explain why we are not verifying the relkind like
heapam_reloptions()?


-               case RELKIND_VIEW:
                case RELKIND_MATVIEW:
+               case RELKIND_VIEW:
                case RELKIND_PARTITIONED_TABLE:

I think this change is unnecessary.

+                       {
+                               Form_pg_class classForm;
+                               HeapTuple       classTup;
+
+                               /* fetch the relation's relcache entry */
+                               if (relation->rd_index->indrelid >= FirstNormalObjectId)
+                               {
+                                       classTup = SearchSysCacheCopy1(RELOID,
ObjectIdGetDatum(relation->rd_index->indrelid));
+                                       classForm = (Form_pg_class) GETSTRUCT(classTup);
+                                       if (classForm->relam >= FirstNormalObjectId)
+                                               tableam = GetTableAmRoutineByAmOid(classForm->relam);
+                                       else
+                                               tableam = GetHeapamTableAmRoutine();
+                                       heap_freetuple(classTup);
+                               }
+                               else
+                               {
+                                       tableam = GetHeapamTableAmRoutine();
+                               }
+                               amoptsfn = relation->rd_indam->amoptions;
+                       }

- We can reduce the indentation by moving the classFrom and classTup into
  the if branch.
- Perhaps we could remove the brace of else branch to maintain consistency
  in the code style.

--
Regards,
Japin Li