Subject: Re: Table AM Interface Enhancements

From: Alexander Korotkov <[email-masked]>
Date: Sun, 3 Mar 2024 13:50:38 +0200

On Mon, Nov 27, 2023 at 10:18 PM Mark Dilger
<[email-masked]> wrote:
>
> > On Nov 25, 2023, at 9:47 AM, Alexander Korotkov <[email-masked]> wrote:
> >
> >> Should the patch at least document which parts of the EState are expected to be in which states, and which parts should be viewed as undefined?  If the implementors of table AMs rely on any/all aspects of EState, doesn't that prevent future changes to how that structure is used?
> >
> > New tuple tuple_insert_with_arbiter() table AM API method needs EState
> > argument to call executor functions: ExecCheckIndexConstraints(),
> > ExecUpdateLockMode(), and ExecInsertIndexTuples().  I think we
> > probably need to invent some opaque way to call this executor function
> > without revealing EState to table AM.  Do you think this could work?
>
> We're clearly not accessing all of the EState, just some specific fields, such as es_per_tuple_exprcontext.  I think you could at least refactor to pass the minimum amount of state information through the table AM API.

Yes, the table AM doesn't need the full EState, just the ability to do
specific manipulation with tuples.  I'll refactor the patch to make a
better isolation for this.

------
Regards,
Alexander Korotkov

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 19 Mar 2024 01:34:13 +0200

> better isolation for this.

Please find the revised patchset attached.  The changes are following:
1. Patchset is rebase. to the current master.
2. Patchset is reordered.  I tried to put less debatable patches to the top.
3. tuple_is_current() method is moved from the Table AM API to the
slot as proposed by Matthias van de Meent.
4. Assert added to the table_free_rd_amcache() as proposed by Pavel Borisov.

------
Regards,
Alexander Korotkov

---

From: Pavel Borisov <[email-masked]>
Date: Tue, 19 Mar 2024 13:34:43 +0400

Hi, Alexander!

On Tue, 19 Mar 2024 at 03:34, Alexander Korotkov <[email-masked]>
wrote:

> > > >> Should the patch at least document which parts of the EState are
> expected to be in which states, and which parts should be viewed as
> undefined?  If the implementors of table AMs rely on any/all aspects of
> EState, doesn't that prevent future changes to how that structure is used?
> > > >
> > > We're clearly not accessing all of the EState, just some specific
> fields, such as es_per_tuple_exprcontext.  I think you could at least
> refactor to pass the minimum amount of state information through the table
> AM API.
> >

Patches 0001-0002 are unchanged compared to the last version in thread [1].
In my opinion, it's still ready to be committed, which was not done for
time were too close to feature freeze one year ago.

0003 - Assert added from previous version. I still have a strong opinion
that allowing multi-chunked data structures instead of single chunks is
completely safe and makes natural process of Postgres improvement that is
self-justified. The patch is simple enough and ready to be pushed.

0004  (previously 0007) - Have not changed, and there is consensus that
this is reasonable. I've re-checked the current code. Looks safe
considering returning a different slot, which I doubted before. So consider
this patch also ready.

0005 (previously 0004) - Unused argument in the is_current_xact_tuple()
signature is removed. Also comparing to v1 the code shifted from tableam
methods to TTS's level.

I'd propose to remove  Assert(!TTS_EMPTY(slot))
for tts_minimal_is_current_xact_tuple()
and  tts_virtual_is_current_xact_tuple() as these are only error reporting
functions that don't use slot actually.

Comment similar to:
+/*
+ * VirtualTupleTableSlots never have a storage tuples.  We generally
+ * shouldn't get here, but provide a user-friendly message if we do.
+ */
also applies to tts_minimal_is_current_xact_tuple()

I'd propose changes for clarity of this comment:
%s/a storage tuples/storage tuples/g
%s/generally//g

Otherwise patch 0005 also looks good to me.

I'm planning to review the remaining patches. Meanwhile think pushing what
is now ready and uncontroversial is a good intention.
Thank you for the work done on this patchset!

Regards,
Pavel Borisov,
Supabase.

[1].
https://www.postgresql.org/message-id/CAPpHfdua-YFw3XTprfutzGp28xXLigFtzNbuFY8yPhqeq6X5kg%40mail.gmail.com

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 19 Mar 2024 15:05:22 +0200

Hi, Pavel!

On Tue, Mar 19, 2024 at 11:34 AM Pavel Borisov <[email-masked]> wrote:
> On Tue, 19 Mar 2024 at 03:34, Alexander Korotkov <[email-masked]> wrote:
>>
> Patches 0001-0002 are unchanged compared to the last version in thread [1]. In my opinion, it's still ready to be committed, which was not done for time were too close to feature freeze one year ago.
>
> 0003 - Assert added from previous version. I still have a strong opinion that allowing multi-chunked data structures instead of single chunks is completely safe and makes natural process of Postgres improvement that is self-justified. The patch is simple enough and ready to be pushed.
>
> 0004  (previously 0007) - Have not changed, and there is consensus that this is reasonable. I've re-checked the current code. Looks safe considering returning a different slot, which I doubted before. So consider this patch also ready.
>
> 0005 (previously 0004) - Unused argument in the is_current_xact_tuple() signature is removed. Also comparing to v1 the code shifted from tableam methods to TTS's level.
>
> I'd propose to remove  Assert(!TTS_EMPTY(slot)) for tts_minimal_is_current_xact_tuple() and  tts_virtual_is_current_xact_tuple() as these are only error reporting functions that don't use slot actually.
>
> Comment similar to:
> +/*
> I'm planning to review the remaining patches. Meanwhile think pushing what is now ready and uncontroversial is a good intention.

Thank you, Pavel!

Regarding 0005, I did apply "a storage tuples" grammar fix.  Regarding
the rest of the things, I'd like to keep methods
tts_*_is_current_xact_tuple() to be similar to nearby
tts_*_getsysattr().  This is why I'm keeping the rest unchanged.  I
think we could refactor that later, but together with
tts_*_getsysattr() methods.

I'm going to push 0003, 0004 and 0005 if there are no objections.

And I'll update 0001 and 0002 in their dedicated thread.

------
Regards,
Alexander Korotkov

---

From: Japin Li <[email-masked]>
Date: Tue, 19 Mar 2024 22:26:06 +0800

> And I'll update 0001 and 0002 in their dedicated thread.

When I try to test the patch on Ubuntu 22.04 with GCC 11.4.0.  There are some
warnings as following:

/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c: In function ‘heapam_acquire_sample_rows’:
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1603:28: warning: implicit declaration of function ‘get_tablespace_maintenance_io_concurrency’ [-Wimplicit-function-declaration]
 1603 |         prefetch_maximum = get_tablespace_maintenance_io_concurrency(onerel->rd_rel->reltablespace);
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1757:30: warning: implicit declaration of function ‘floor’ [-Wimplicit-function-declaration]
 1757 |                 *totalrows = floor((liverows / bs.m) * totalblocks + 0.5);
      |                              ^~~~~
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:49:1: note: include ‘<math.h>’ or provide a declaration of ‘floor’
   48 | #include "utils/sampling.h"
  +++ |+#include <math.h>
   49 |
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1757:30: warning: incompatible implicit declaration of built-in function ‘floor’ [-Wbuiltin-declaration-mismatch]
 1757 |                 *totalrows = floor((liverows / bs.m) * totalblocks + 0.5);
      |                              ^~~~~
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1757:30: note: include ‘<math.h>’ or provide a declaration of ‘floor’
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1603:21: warning: implicit declaration of function 'get_tablespace_maintenance_io_concurrency' is invalid in C99 [-Wimplicit-function-declaration]
        prefetch_maximum = get_tablespace_maintenance_io_concurrency(onerel->rd_rel->reltablespace);
                           ^

It seems you forgot to include math.h and utils/spccache.h header files
in heapam_handler.c.

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index ac24691bd2..04365394f1 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -19,6 +19,8 @@
  */
 #include "postgres.h"

+#include <math.h>
+
 #include "access/genam.h"
 #include "access/heapam.h"
 #include "access/heaptoast.h"
@@ -46,6 +48,7 @@
 #include "utils/builtins.h"
 #include "utils/rel.h"
 #include "utils/sampling.h"
+#include "utils/spccache.h"

 static TM_Result heapam_tuple_lock(Relation relation, Datum tupleid,
 								   Snapshot snapshot, TupleTableSlot *slot,

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 19 Mar 2024 17:28:41 +0200

> When I try to test the patch on Ubuntu 22.04 with GCC 11.4.0.  There are some

Thank you for catching this!
Please, find the revised patchset attached.

------
Regards,
Alexander Korotkov

---

From: Pavel Borisov <[email-masked]>
Date: Wed, 20 Mar 2024 09:22:47 +0400

Hi, Alexander!

For 0007:

Code inside

+heapam_reloptions(char relkind, Datum reloptions, bool validate)
+{
+   if (relkind == RELKIND_RELATION ||
+       relkind == RELKIND_TOASTVALUE ||
+       relkind == RELKIND_MATVIEW)
+       return heap_reloptions(relkind, reloptions, validate);
+
+   return NULL;

looks redundant to what is done inside heap_reloptions(). Was this on
purpose? Is it possible to leave only "return heap_reloptions()"  ?

This looks like a duplicate:
src/include/access/reloptions.h:extern bytea
*index_reloptions(amoptions_function amoptions, Datum reloptions,
src/include/access/tableam.h:extern bytea
*index_reloptions(amoptions_function amoptions, Datum reloptions,

Otherwise the patch looks good and doing what it's proposed to do.

Regards,
Pavel Borisov.

---

From: Pavel Borisov <[email-masked]>
Date: Thu, 21 Mar 2024 09:36:14 +0400

Hi, Alexander!

> Otherwise the patch looks good and doing what it's proposed to do.

For patch 0006:

The change for analyze is in the same style as for previous table am
extensibility patches.

table_scan_analyze_next_tuple/table_scan_analyze_next_block existing
extensibility is dropped in favour of more general method
table_relation_analyze. I haven't found existing extensions on a GitHub
that use these table am's, so probably it's quite ok to remove the
extensibility that didn't get any traction for many years.

The patch contains a big block of code copy-paste. I've checked that the
code is the same with only function name replacement in favor to using
table am instead of heap am. I'd propose restoring the static functions
declaration in the head of the file, which was removed in the patch and
place heapam_acquire_sample_rows() above compare_rows() to make functions
copied as the whole code block. This is for better patch look only, not a
principal change.

-static int acquire_sample_rows(Relation onerel, int elevel,
-                               HeapTuple *rows, int targrows,
-                               double *totalrows, double *totaldeadrows);
-static int compare_rows(const void *a, const void *b, void *arg)

May it also be a better place than vacuum.h for
typedef int (*AcquireSampleRowsFunc) ? Maybe sampling.h ?

The other patch that I'd like to review is 0012:

For a
typedef enum RowRefType
 I think some comments would be useful to avoid confusion about the changes
like
-               newrc->allMarkTypes = (1 << newrc->markType);
+              newrc->allRefTypes = (1 << refType);

Also I think the semantical difference between ROW_REF_COPY
and ROW_MARK_COPY is better to be mentioned in the comments and/or commit
message. This may include a description of assigning different reftypes in
parse_relation.c

In a comment there is a small confusion between markType and refType:

 * The parent's allRefTypes field gets the OR of (1<<refType) across all
 * its children (this definition allows children to use different
markTypes).

Both patches look good to me and are ready, though they may need minimal
comments/cosmetic work.

Regards,
Pavel Borisov

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 22 Mar 2024 08:51:31 +0400

Hi, Alexander!

Thank you for working on this patchset and pushing some of these patches!

I tried to write comments for tts_minimal_is_current_xact_tuple()
and tts_minimal_getsomeattrs() for them to be the same as for the same
functions for heap and virtual tuple slots, as I proposed above in the
thread. (tts_minimal_getsysattr is not introduced by the current patchset,
but anyway)

Meanwhile I found that (never appearing) error message
for tts_minimal_is_current_xact_tuple needs to be corrected. Please see the
patch in the attachment.

Regards,
Pavel Borisov

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 22 Mar 2024 08:56:33 +0400

On Fri, 22 Mar 2024 at 08:51, Pavel Borisov <[email-masked]> wrote:

> I need to correct myself: it's for  tts_minimal_getsysattr() not
tts_minimal_getsomeattrs()

Pavel.

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 27 Mar 2024 01:22:35 +0200

On Fri, Mar 22, 2024 at 6:56 AM Pavel Borisov <[email-masked]> wrote:
>> I tried to write comments for tts_minimal_is_current_xact_tuple() and tts_minimal_getsomeattrs() for them to be the same as for the same functions for heap and virtual tuple slots, as I proposed above in the thread. (tts_minimal_getsysattr is not introduced by the current patchset, but anyway)
>>
>> Meanwhile I found that (never appearing) error message for tts_minimal_is_current_xact_tuple needs to be corrected. Please see the patch in the attachment.
>>
> I need to correct myself: it's for  tts_minimal_getsysattr() not tts_minimal_getsomeattrs()

Pushed.

The revised rest of the patchset is attached.
0001 (was 0006) – I prefer the definition of AcquireSampleRowsFunc to
stay in vacuum.h.  If we move it to sampling.h then we would have to
add there includes to define Relation, HeapTuple etc.  I'd like to
avoid this kind of change.  Also, I've deleted
table_beginscan_analyze(), because it's only called from
tableam-specific AcquireSampleRowsFunc.  Also I put some comments to
heapam_scan_analyze_next_block() and heapam_scan_analyze_next_tuple()
given that there are now no relevant comments for them in tableam.h.
I've removed some redundancies from acquire_sample_rows().  And added
comments to AcquireSampleRowsFunc based on what we have in FDW docs
for this function.  Did some small edits as well.  As you suggested,
turned back declarations for acquire_sample_rows() and compare_rows().

0002 (was 0007) – I've turned the redundant "if", which you've pointed
out, into an assert.  Also, added some comments, most notably comment
for TableAmRoutine.reloptions based on the indexam docs.

0007 (was 0012) – This patch doesn't make much sense if not removing
ROW_MARK_COPY.  What an oversight by me!  I managed to remove
ROW_MARK_COPY so that tests passed.  Added a lot of comments and made
other improvements.  But the problem is that I didn't manage to
research all the consequences of this patch to FDW.  And I think there
are open design questions.  In particular how should ROW_REF_COPY work
with row marks other than ROW_MARK_REFERENCE and should it work at
all?  This would require some consensus, and it doesn't seem feasible
to achieve before FF.  So, I think this is not a subject for v17.

Other patches are without changes.

------
Regards,
Alexander Korotkov

---

From: Pavel Borisov <[email-masked]>
Date: Wed, 27 Mar 2024 16:51:54 +0400

Hi, Alexander!

> turned back declarations for acquire_sample_rows() and compare_rows().

In my comment in the thread I was not thinking about returning the old name
acquire_sample_rows(), it was only about the declarations and the order of
functions to be one code block. To me heapam_acquire_sample_rows() looks
better for a name of heap implementation of *AcquireSampleRowsFunc(). I
suggest returning the name heapam_acquire_sample_rows() from v4. Sorry for
the confusion in this.

The changed type of static function that always returned true for heap
looks good to me:
static void heapam_scan_analyze_next_block

The same is for removing the comparison of always true "block_accepted" in
(heapam_)acquire_sample_rows()

Removing table_beginscan_analyze and call scan_begin() is not in the same
style as other table_beginscan_* functions. Though this is not a change in
functionality, I'd leave this part as it was in v4. Also, a comment about
it was introduced in v5:

src/backend/access/heap/heapam_handler.c: * with table_beginscan_analyze()

For comments I'd propose:
%s/In addition, store estimates/In addition, a function should store
estimates/g
%s/zerp/zero/g

> for TableAmRoutine.reloptions based on the indexam docs.

%s/validate sthe/validates the/g

This seems not needed, it's already inited to InvalidOid before.
+else
+accessMethod = default_table_access_method;

+       accessMethodId = InvalidOid;

This code came from 374c7a22904. I don't insist on this simplification in a
patch 0002.

Overall both patches look good to me.

Regards,
Pavel Borisov.

---

From: Pavel Borisov <[email-masked]>
Date: Wed, 27 Mar 2024 16:54:51 +0400

> This code came from 374c7a22904. I don't insist on this simplification in
> a patch 0002.
>

A correction of the code quote for the previous message:

+else

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 28 Mar 2024 00:14:32 +0200

Hi, Pavel!

Thank you for your feedback.  The revised patch set is attached.

I found that vacuum.c has a lot of heap-specific code.  Thus, it
should be OK for analyze.c to keep heap-specific code.  Therefore, I
rolled back the movement of functions between files.  That leads to a
smaller patch, easier to review.

On Wed, Mar 27, 2024 at 2:52 PM Pavel Borisov <[email-masked]> wrote:
> In my comment in the thread I was not thinking about returning the old name acquire_sample_rows(), it was only about the declarations and the order of functions to be one code block. To me heapam_acquire_sample_rows() looks better for a name of heap implementation of *AcquireSampleRowsFunc(). I suggest returning the name heapam_acquire_sample_rows() from v4. Sorry for the confusion in this.

I found that the function name acquire_sample_rows is referenced in
quite several places in the source code.  So, I would prefer to save
the old name to keep the changes minimal.

> The changed type of static function that always returned true for heap looks good to me:
> The same is for removing the comparison of always true "block_accepted" in (heapam_)acquire_sample_rows()

Ok!

> Removing table_beginscan_analyze and call scan_begin() is not in the same style as other table_beginscan_* functions. Though this is not a change in functionality, I'd leave this part as it was in v4.

With the patch, this method doesn't have usages outside of table am.
I don't think we should keep a method, which doesn't have clear
external usage patterns.  But I agree that starting a scan with
heap_beginscan() and ending with table_endscan() is not correct.  Now
ending this scan with heap_endscan().

> Also, a comment about it was introduced in v5:
>

Corrected.

> %s/In addition, store estimates/In addition, a function should store estimates/g
> %s/zerp/zero/g

Fixed.

> %s/validate sthe/validates the/g

Fixed.

> This code came from 374c7a22904. I don't insist on this simplification in a patch 0002.

This is minor redundancy.  I prefer to keep it.  This makes it obvious
that patch just moved this code.

------
Regards,
Alexander Korotkov

---

From: Pavel Borisov <[email-masked]>
Date: Thu, 28 Mar 2024 13:46:30 +0400

Hi, Alexander!
Thank you for working on these patches.
On Thu, 28 Mar 2024 at 02:14, Alexander Korotkov <[email-masked]>

> smaller patch, easier to review.
I agree with you.
And with the changes remaining in heapam_handler.c I suppose we can also
remove the includes introduced:

#include <math.h>
#include "utils/sampling.h"
#include "utils/spccache.h"

On Wed, Mar 27, 2024 at 2:52 PM Pavel Borisov <[email-masked]>
> > In my comment in the thread I was not thinking about returning the old
> name acquire_sample_rows(), it was only about the declarations and the
> order of functions to be one code block. To me heapam_acquire_sample_rows()
> looks better for a name of heap implementation of *AcquireSampleRowsFunc().
> I suggest returning the name heapam_acquire_sample_rows() from v4. Sorry
> for the confusion in this.
>
The full list shows only a couple of changes in analyze.c and several
comments elsewhere.

contrib/postgres_fdw/postgres_fdw.c:             * of the relation.  Same
algorithm as in acquire_sample_rows in
src/backend/access/heap/vacuumlazy.c:            * match what analyze.c's
acquire_sample_rows() does, otherwise VACUUM
src/backend/access/heap/vacuumlazy.c:            * The logic here is a bit
simpler than acquire_sample_rows(), as
src/backend/access/heap/vacuumlazy.c:                            * what
acquire_sample_rows() does.
src/backend/access/heap/vacuumlazy.c:                            *
acquire_sample_rows() does, so be consistent.
src/backend/access/heap/vacuumlazy.c:            * acquire_sample_rows()
will recognize the same LP_DEAD items as dead
src/backend/commands/analyze.c:static int
acquire_sample_rows(Relation onerel, int elevel,
src/backend/commands/analyze.c:         acquirefunc = acquire_sample_rows;
src/backend/commands/analyze.c: * acquire_sample_rows -- acquire a random
sample of rows from the table
src/backend/commands/analyze.c:acquire_sample_rows(Relation onerel, int
elevel,
src/backend/commands/analyze.c: * This has the same API as
acquire_sample_rows, except that rows are
src/backend/commands/analyze.c:                 acquirefunc =
acquire_sample_rows;

My point for renaming is to make clear that it's a heap implementation of
acquire_sample_rows which could be useful for possible reworking heap
implementations of table am methods into a separate place later. But I'm
also ok with the existing naming.

> > Removing table_beginscan_analyze and call scan_begin() is not in the
> same style as other table_beginscan_* functions. Though this is not a
> change in functionality, I'd leave this part as it was in v4.
>
Good!

> that patch just moved this code.
I agree with the remaining.

Regards,
Pavel Borisov

---

From: Pavel Borisov <[email-masked]>
Date: Thu, 28 Mar 2024 17:12:20 +0400

Hi, Alexander!

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.

Maybe, a commit message is also better to reformulate to describe better
who should do what.

I think, with rebase and correction in the comments/commit message patch
0006 is ready to be committed.

Regards,
Pavel Borisov.

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 28 Mar 2024 15:26:07 +0200

Hi Pavel!

Revised patchset is attached.

On Thu, Mar 28, 2024 at 3:12 PM Pavel Borisov <[email-masked]> 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.
>
> 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

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.

------
Regards,
Alexander Korotkov

---

From: Japin Li <[email-masked]>
Date: Thu, 28 Mar 2024 23:23:05 +0800

> 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

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 29 Mar 2024 13:33:30 +0400

I found that after yesterday's e2395cdbe83a 0002 don't apply.
Rebased the whole patchset.

Pavel

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 29 Mar 2024 18:07:44 +0400

I've looked at patch 0003.

Generally, it does a similar thing as 0001 - it exposes a more generalized
method tuple_insert_with_arbiter that encapsulates
tuple_insert_speculative/tuple_complete_speculative and at the same time
allows extensibility of this i.e. different implementation for custom table
other than heap by the extensions. Though the code rearrangement is little
bit more complicated, the patch is clear. It doesn't change the behavior
for heap tables.

tuple_insert_speculative/tuple_complete_speculative are removed from table
AM methods. I think it would not be hard for existing users of this
to adapt to the changes.

Code block ExecCheckTupleVisible -- ExecCheckTIDVisible moved
to heapam_handler.c I've checked, the code block unchanged except
that ExecCheckTIDVisible now gets Relation from the caller instead of
constructing it from ResultRelInfo.

Also two big code blocks are moved from ExecOnConflictUpdate and ExecInsert
to a new method heapam_tuple_insert_with_arbiter. They correspond the old
code with several minor modifications.

For ExecOnConflictUpdate comment need to be revised. This one is for
shifted code:
> * Try to lock tuple for update as part of speculative insertion.
Probably it is worth to be moved to a comment for
heapam_tuple_insert_with_arbiter.

For heapam_tuple_insert_with_arbiter both "return NULL" could be shifted
level up into the end of the block:
>if (!ExecCheckIndexConstraints(resultRelInfo, slot, estate, &conflictTid,
>+
 arbiterIndexes))

Also I'd add comment for heapam_tuple_insert_with_arbiter:
/* See comments for table_tuple_insert_with_arbiter() */

A comment to be corrected:
src/backend/access/heap/heapam.c: * implement
table_tuple_insert_speculative()

As Jaipin said, I'd also propose removing "inline"
from heapam_tuple_insert_with_arbiter.

More corrections for comments:
%s/If tuple doesn't violates/If tuple doesn't violate/g
%s/which comprises the list of/list, which comprises/g
%s/conflicting tuple gets locked/conflicting tuple should be locked/g

I think for better code look this could be removed:
>vlock:
 >                       CHECK_FOR_INTERRUPTS();
together with CHECK_FOR_INTERRUPTS(); in heapam_tuple_insert_with_arbiter
placed in the beginning of while loop.

Overall the patch looks good enough to me.

Regards,
Pavel

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 29 Mar 2024 19:23:00 +0400

> placed in the beginning of while loop.
To clarify things, this I wrote only about CHECK_FOR_INTERRUPTS();
rearrangement.

Regards,
Pavel