On Thu, 28 Mar 2024 at 21:26, Alexander Korotkov <[email protected]> wrote:
> Hi Pavel!
>
> Revised patchset is attached.
>
> On Thu, Mar 28, 2024 at 3:12 PM Pavel Borisov <[email protected]> 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 table access methods could redefine this (only for tables created with
>> these am's) and make index inserts independently of ExecInsertIndexTuples
>> 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
