Subject: Re: New Table Access Methods for Multi and Single Inserts

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 21 Mar 2024 09:44:16 +0530

On Tue, Mar 19, 2024 at 10:40 AM Masahiko Sawada <[email-masked]> wrote:
>
> I've not reviewed the patches in depth yet, but run performance tests
> for CREATE MATERIALIZED VIEW. The test scenarios is:

Thanks for looking into this.

> Is there any reason why we copy a buffer-heap tuple to another
> buffer-heap tuple? Which results in that we increments the buffer
> refcount and register it to ResourceOwner for every tuples. I guess
> that the destination tuple slot is not necessarily a buffer-heap, and
> we could use VirtualTupleTableSlot instead. It would in turn require
> copying a heap tuple. I might be missing something but it improved the
> performance at least in my env. The change I made was:
>
> -       dstslot = table_slot_create(state->rel, NULL);
> +       //dstslot = table_slot_create(state->rel, NULL);
> +       dstslot = MakeTupleTableSlot(RelationGetDescr(state->rel),
> +                                    &TTSOpsVirtual);
> +
>
> And the execution times are:
> 1588.984 ms
> 1591.618 ms
> 1582.519 ms

Yes, usingVirtualTupleTableSlot helps improve the performance a lot.
Below are results from my testing. Note that CMV, RMV, CTAS stand for
CREATE MATERIALIZED VIEW, REFRESH MATERIALIZED VIEW, CREATE TABLE AS
respectively. These commands got faster by 62.54%, 68.87%, 74.31% or
2.67, 3.21, 3.89 times respectively. I've used the test case specified
at [1].

HEAD:
CMV:
Time: 6276.468 ms (00:06.276)
CTAS:
Time: 8141.632 ms (00:08.142)
RMV:
Time: 14747.139 ms (00:14.747)

PATCHED:
CMV:
Time: 2350.282 ms (00:02.350)
CTAS:
Time: 2091.427 ms (00:02.091)
RMV:
Time: 4590.180 ms (00:04.590)

I quickly looked at the description of what a "virtual" tuple is from
src/include/executor/tuptable.h [2]. IIUC, it is invented for
minimizing data copying, but it also says that it's the responsibility
of the generating plan node to be sure these resources are not
released for as long as the virtual tuple needs to be valid or is
materialized. While it says this, as far as this patch is concerned,
the virtual slot gets materialized when we copy the tuples from source
slot (can be any type of slot) to destination slot (which is virtual
slot). See ExecCopySlot->
tts_virtual_copyslot->tts_virtual_materialize. This way,
tts_virtual_copyslot ensures the tuples storage doesn't depend on
external memory because all the datums that aren't passed by value are
copied into the slot's memory context.

With the above understanding, it looks safe to use virtual slots for
the multi insert buffered slots. I'm not so sure if I'm missing
anything here.

[1]
cd $PWD/pg17/bin
rm -rf data logfile
./initdb -D data
./pg_ctl -D data -l logfile start

./psql -d postgres
\timing
drop table test cascade;
create unlogged table test (c int);
insert into test select generate_series(1, 10000000);
create materialized view test_mv as select * from test;
create table test_copy as select * from test;
insert into test select generate_series(1, 10000000);
refresh materialized view test_mv;

[2]
 * A "virtual" tuple is an optimization used to minimize physical data copying
 * in a nest of plan nodes.  Until materialized pass-by-reference Datums in
 * the slot point to storage that is not directly associated with the
 * TupleTableSlot; generally they will point to part of a tuple stored in a
 * lower plan node's output TupleTableSlot, or to a function result
 * constructed in a plan node's per-tuple econtext.  It is the responsibility
 * of the generating plan node to be sure these resources are not released for
 * as long as the virtual tuple needs to be valid or is materialized.  Note
 * also that a virtual tuple does not have any "system columns".

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 21 Mar 2024 13:10:16 +0530

> the multi insert buffered slots. I'm not so sure if I'm missing

I'm attaching the v13 patches using virtual tuple slots for buffered
tuples for multi inserts.

---

From: Jeff Davis <[email-masked]>
Date: Fri, 22 Mar 2024 17:17:12 -0700

> tuples for multi inserts.

Comments:

* Do I understand correctly that CMV, RMV, and CTAS experience a
performance benefit, but COPY FROM does not? And is that because COPY
already used table_multi_insert, whereas CMV and RMV did not?

* In the COPY FROM code, it looks like it's deciding whether to flush
based on MAX_BUFFERED_TUPLES, but the slot array is allocated with
MAX_BUFFERED_SLOTS (they happen to be the same for heap, but perhaps
not for other AMs). The copy code shouldn't be using internal knowledge
of the multi-insert code; it should know somehow from the API when the
right time is to flush.

* How is the memory management expected to work? It looks like COPY
FROM is using the ExprContext when running the input functions, but we
really want to be using a memory context owned by the table AM, right?

* What's the point of the table_multi_insert_slots() "num_slots"
argument? The only caller simply discards it.

* table_tuple_insert_v2 isn't called anywhere, what's it for?

* the "v2" naming is inconsistent -- it seems you only added it in
places where there's a name conflict, which makes it hard to tell which
API methods go together. I'm not sure how widely table_multi_insert* is
used outside of core, so it's possible that we may even be able to just
change those APIs and the few extensions that call it can be updated.

* Memory usage tracking should be done in the AM by allocating
everything in a single context so it's easy to check the size. Don't
manually add up memory.

* I don't understand: "Caller may have got the slot using
heap_multi_insert_next_free_slot, filled it and passed. So, skip
copying in such a case." If the COPY FROM had a WHERE clause and
skipped a tuple after filling the slot, doesn't that mean the slot has
bogus data from the last tuple?

* We'd like this to work for insert-into-select (IIS) and logical
replication, too. Do you see any problem there, or is it just a matter
of code?

* Andres had some comments[1] that don't seem entirely addressed.
  - You are still allocating the AM-specific part of TableModifyState
as a separately-allocated chunk.
  - It's still called TableInsertState rather than TableModifyState as
he suggested. If you change that, you should also change to
table_modify_begin/end.
  - CID: I suppose Andres is considering the use case of "BEGIN; ...
ten thousand inserts ... COMMIT;". I don't think this problem is really
solvable (discussed below) but we should have some response/consensus
on that point.
  - He mentioned that we only need one new method implemented by the
AM. I don't know if one is enough, but 7 does seem excessive. I have
some simplification ideas below.

Overall:

If I understand correctly, there are two ways to use the API:

1. used by CTAS, MV:

  tistate = table_insert_begin(...);
  table_multi_insert_v2(tistate, tup1);
  ...
  table_multi_insert_v2(tistate, tupN);
  table_insert_end(tistate);

2. used by COPY ... FROM:

  tistate = table_insert_begin(..., SKIP_FLUSH);
  if (multi_insert_slot_array_is_full())
     table_multi_insert_flush(tistate);
  slot = table_insert_next_free_slot(tistate);
  ... fill slot with tup1
  table_multi_insert_v2(tistate, tup1);
  ...
  slot = table_insert_next_free_slot(tistate);
  ... fill slot with tupN
  table_multi_insert_v2(tistate, tupN);
  table_insert_end(tistate);

Those two uses need comments explaining what's going on. It appears the
SKIP_FLUSH flag is used to indicate which use the caller intends.

Use #2 is not enforced well by either the API or runtime checks. If the
caller neglects to check for a full buffer, it appears that it will
just overrun the slots array.

Also, for use #2, table_multi_insert_v2() doesn't do much other than
incrementing the memory used. The slot will never be NULL because it
was obtained with table_multi_insert_next_free_slot(), and the other
two branches don't happen when SKIP_FLUSH is true.

The real benefit to COPY of your new API is that the AM can manage
slots for itself, and how many tuples may be tracked (which might be a
lot higher for non-heap AMs).

I agree with Luc Vlaming's comment[2] that more should be left to the
table AM. Your patch tries too hard to work with the copyfrom.c slot
array, somehow sharing it with the table AM. That adds complexity to
the API and feels like a layering violation.

We also shouldn't mandate a slot array in the API. Each slot is 64
bytes -- a lot of overhead for small tuples. For a non-heap AM, it's
much better to store the tuple data in a big contiguous chunk with
minimal overhead.

Let's just have a simple API like:

  tmstate = table_modify_begin(...);
  table_modify_save_insert(tmstate, tup1);
  ...
  table_modify_save_insert(tmstate, tupN);
  table_modify_end(tmstate);

and leave it up to the AM to do all the buffering and flushing work (as
Luc Vlaming suggested[2]).

That leaves one problem, which is: how do we update the indexes and
call AR triggers while flushing? I think the best way is to just have a
callback in the TableModifyState that is called during flush. (I don't
think that would affect performance, but worth double-checking.)

We have to disable this whole multi-insert mechanism if there are
volatile BR/AR triggers, because those are supposed to see already-
inserted tuples. That's not a problem with your patch but it is a bit
unfortunate -- triggers can be costly already, but this increases the
penalty. There may be some theoretical ways to avoid this problem, like
reading tuples out of the unflushed buffer during a SELECT, which
sounds a little too clever (though perhaps not completely crazy if the
AM is in control of both?).

For potentially working with multi-updates/deletes, it might be as
simple as tracking the old TIDs along with the slots and having new
_save_update and _save_delete methods. I haven't thought deeply about
that, and I'm not sure we have a good example AM to work with, but it
seems plausible that we could make something useful here.

To batch multiple different INSERT statements within a transaction just
seems like a really hard problem. That could mean different CIDs, but
also different subtransaction IDs. Constraint violation errors will
happen at the time of flushing, which could be many commands later from
the one that actually violates the constraint. And what if someone
issues a SELECT in the middle of the transaction, how does it see the
already-inserted-but-not-flushed tuples? If that's not hard enough
already, then you would also need to extend low-level APIs to accept
arbitrary CIDs and subxact IDs when storing tuples during a flush. The
only way I could imagine solving all of these problems is declaring
somehow that your transaction won't do any of these complicated things,
and that you don't mind getting constraint violations at the wrong
time. So I recommend that you punt on this problem.

Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/20230603223824.o7iyochli2dwwi7k%40alap3.anarazel.de
[2]
https://www.postgresql.org/message-id/508af801-6356-d36b-1867-011ac6df8f55%40swarm64.com

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 26 Mar 2024 01:28:00 +0530

> Comments:

Thanks for looking into it.

> already used table_multi_insert, whereas CMV and RMV did not?

Yes, that's right. COPY FROM is already optimized with multi inserts.

I now have a feeling that I need to simplify the patches. I'm thinking
of dropping the COPY FROM patch using the new multi insert API for the
following reasons:
1. We can now remove some of the new APIs (table_multi_insert_slots
and table_multi_insert_next_free_slot) that were just invented for
COPY FROM.
2. COPY FROM is already optimized with multi inserts, so no real gain
is expected with the new multi insert API.
3. As we are inching towards feature freeze, simplifying the patches
by having only the necessary things increases the probability of
getting this in.
4. The real benefit of this whole new multi insert API is seen if used
for the commands CMV, RMV, CTAS. These commands got faster by 62.54%,
68.87%, 74.31% or 2.67, 3.21, 3.89 times respectively.
5. This leaves with really simple APIs. No need for callback stuff for
dealing with indexes, triggers etc. as CMV, RMV, CTAS cannot have any
of them.

The new APIs are more extensible, memory management is taken care of
by AM, and with TableModifyState as the structure name and more
meaningful API names. The callback for triggers/indexes etc. aren't
taken care of as I'm now only focusing on CTAS, CMV, RMV
optimizations.

Please see the attached v14 patches.

---

From: Jeff Davis <[email-masked]>
Date: Tue, 26 Mar 2024 08:37:04 -0700

On Tue, 2024-03-26 at 01:28 +0530, Bharath Rupireddy wrote:
> I'm thinking
> following reasons: ...

I agree with all of this. We do want COPY ... FROM support, but there
are some details to work out and we don't want to make a big code
change at this point in the cycle.

> Please see the attached v14 patches.

* No need for a 'kind' field in TableModifyState. The state should be
aware of the kinds of changes that it has received and that may need to
be flushed later -- for now, only inserts, but possibly updates/deletes
in the future.

* If the AM doesn't support the bulk methods, fall back to retail
inserts instead of throwing an error.

* It seems like this API will eventually replace table_multi_insert and
table_finish_bulk_insert completely. Do those APIs have any advantage
remaining over the new one proposed here?

* Right now I don't any important use of the flush method. It seems
that could be accomplished in the finish method, and flush could just
be an internal detail when the memory is exhausted. If we find a use
for it later, we can always add it, but right now it seems unnecessary.

* We need to be careful about cases where the command can be successful
but the writes are not flushed. I don't tihnk that's a problem with the
current patch, but we will need to do something here when we expand to
INSERT INTO ... SELECT.

Andres, is this patch overall closer to what you had in mind in the
email here:

https://www.postgresql.org/message-id/[email-masked]

?

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 27 Mar 2024 01:19:51 +0530

> change at this point in the cycle.

Right.

> be flushed later -- for now, only inserts, but possibly updates/deletes

Removed 'kind' field with lazy initialization of required AM specific
modify (insert in this case) state. Since we don't have 'kind', I
chose the callback approach to cleanup the modify (insert in this
case) specific state at the end.

> inserts instead of throwing an error.

For instance, CREATE MATERIALIZED VIEW foo_mv AS SELECT * FROM foo
USING bar_tam; doesn't work if bar_tam doesn't have the
table_tuple_insert implemented.

Similarly, with this new AM, the onus lies on the table AM
implementers to provide an implementation for these new AMs even if
they just do single inserts. But, I do agree that we must catch this
ahead during parse analysis itself, so I've added assertions in
GetTableAmRoutine().

> remaining over the new one proposed here?

table_multi_insert needs to be there for sure as COPY ... FROM uses
it. Not sure if we need to remove the optional callback
table_finish_bulk_insert though. Heap AM doesn't implement one, but
some other AM might. Having said that, with this new AM, whatever the
logic that used to be there in table_finish_bulk_insert previously,
table AM implementers will have to move them to table_modify_end.

FWIW, I can try writing a test table AM that uses this new AM but just
does single inserts, IOW, equivalent to table_tuple_insert().
Thoughts?

> for it later, we can always add it, but right now it seems unnecessary.

Firstly, we are not storing CommandId and options in TableModifyState,
because we expect CommandId to be changing (per Andres comment).
Secondly, we don't want to pass just the CommandId and options to
table_modify_end(). Thirdly, one just has to call the
table_modify_buffer_flush before the table_modify_end. Do you have any
other thoughts here?

> current patch, but we will need to do something here when we expand to

You mean, writes are not flushed to the disk? Can you please elaborate
why it's different for INSERT INTO ... SELECT and not others? Can't
the new flush AM be helpful here to implement any flush related
things?

Please find the attached v15 patches with the above review comments addressed.

---

From: Jeff Davis <[email-masked]>
Date: Wed, 27 Mar 2024 01:12:19 -0700

On Wed, 2024-03-27 at 01:19 +0530, Bharath Rupireddy wrote:
> they just do single inserts.

Why not fall back to using the plain tuple_insert? Surely some table
AMs might be simple and limited, and we shouldn't break them just
because they don't implement the new APIs.

> it.

After we have these new APIs fully in place and used by COPY, what will
happen to those other APIs? Will they be deprecated or will there be a
reason to keep them?

> does single inserts, IOW, equivalent to table_tuple_insert().

More table AMs to test against would be great, but I also know that can
be a lot of work.

> because we expect CommandId to be changing (per Andres comment).

Trying to make this feature work across multiple commands poses a lot
of challenges: what happens when there are SELECTs and subtransactions
and non-deferrable constraints?

Regardless, if we care about multiple CIDs, they should be stored along
with the tuples, not supplied at the time of flushing.

> the new flush AM be helpful here to implement any flush related

Not a major problem. We can discuss while working on IIS support.

I am concnerned that the flush callback is not a part of the API. We
will clearly need that to support index insertions for COPY/IIS, so as-
is the API feels incomplete. Thoughts?

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Sun, 31 Mar 2024 21:18:14 +0530

> because they don't implement the new APIs.

Hm. That might complicate table_modify_begin,
table_modify_buffer_insert and table_modify_end a bit. What do we put
in TableModifyState then? Do we create the bulk insert state
(BulkInsertStateData) outside? I think to give a better interface, can
we let TAM implementers support these new APIs in their own way? If
this sounds rather intrusive, we can just implement the fallback to
tuple_insert if these new API are not supported in the caller, for
example, do something like below in createas.c and matview.c.
Thoughts?

if (table_modify_buffer_insert() is defined)
   table_modify_buffer_insert(...);
else
{
  myState->bistate = GetBulkInsertState();
  table_tuple_insert(...);
}

> happen to those other APIs? Will they be deprecated or will there be a

Deprecated perhaps?

Please find the attached v16 patches for further review.

---

From: Jeff Davis <[email-masked]>
Date: Tue, 02 Apr 2024 12:40:36 -0700

>   myState->bistate = GetBulkInsertState();

We can't alloc/free the bulk insert state for every insert call. I see
two options:

* Each caller needs to support two code paths: if the buffered insert
APIs are defined, then use those; otherwise the caller needs to manage
the bulk insert state itself and call the plain insert API.

* Have default implementation for the new API methods, so that the
default for the begin method would allocate the bulk insert state, and
the default for the buffered insert method would be to call plain
insert using the bulk insert state.

I'd prefer the latter, at least in the long term. But I haven't really
thought through the details, so perhaps we'd need to use the former.

> > happen to those other APIs? Will they be deprecated or will there

Including Alexander on this thread, because he's making changes to the
multi-insert API. We need some consensus on where we are going with
these APIs before we make more changes, and what incremental steps make
sense in v17.

Here's where I think this API should go:

1. Have table_modify_begin/end and table_modify_buffer_insert, like
those that are implemented in your patch.

2. Add some kind of flush callback that will be called either while the
tuples are being flushed or after the tuples are flushed (but before
they are freed by the AM). (Aside: do we need to call it while the
tuples are being flushed to get the right visibility semantics for
after-row triggers?)

3. Add table_modify_buffer_{update|delete} APIs.

4. Some kind of API tweaks to help manage memory when modifying
pertitioned tables, so that the buffering doesn't get out of control.
Perhaps just reporting memory usage and allowing the caller to force
flushes would be enough.

5. Use these new methods for CREATE/REFRESH MATERIALIZED VIEW. This is
fairly straightforward, I believe, and handled by your patch. Indexes
are (re)built afterward, and no triggers are possible.

6. Use these new methods for CREATE TABLE ... AS. This is fairly
straightforward, I believe, and handled by your patch. No indexes or
triggers are possible.

7. Use these new methods for COPY. We have to be careful to avoid
regressions for the heap method, because it's already managing its own
buffers. If the AM manages the buffering, then it may require
additional copying of slots, which could be a disadvantage. To solve
this, we may need some minor API tweaks to avoid copying when the
caller guarantees that the memory will not be freed to early, or
perhaps expose the AM's memory context to copyfrom.c. Another thing to
consider is that the buffering in copyfrom.c is also used for FDWs, so
that buffering code path needs to be preserved in copyfrom.c even if
not used for AMs.

8. Use these new methods for INSERT INTO ... SELECT. One potential
challenge here is that execution nodes are not always run to
completion, so we need to be sure that the flush isn't forgotten in
that case.

9. Use these new methods for DELETE, UPDATE, and MERGE. MERGE can use
the buffer_insert/update/delete APIs; we don't need a separate merge
method. This probably requires that the AM maintain 3 separate buffers
to distinguish different kinds of changes at flush time (obviously
these can be initialized lazily to avoid overhead when not being used).

10. Use these new methods for logical apply.

11. Deprecate the multi_insert API.

Thoughts on this plan? Does your patch make sense in v17 as a stepping
stone, or should we try to make all of these API changes together in
v18?

Also, a sample AM code would be a huge benefit here. Writing a real AM
is hard, but perhaps we can at least have an example one to demonstrate
how to use these APIs?

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 3 Apr 2024 14:32:43 +0530

> thought through the details, so perhaps we'd need to use the former.

I too prefer the latter so that the caller doesn't have to have two
paths. The new API can just transparently fallback to single inserts.
I've implemented that in the attached v17 patch. I also tested the
default APIs manually, but I'll see if I can add some tests to it the
default API.

> stone, or should we try to make all of these API changes together in

I'd like to see the new multi insert API (as proposed in the v17
patches) for PG17 if possible. The basic idea with these new APIs is
to let the AM implementers choose the right buffered insert strategy
(one can choose the AM specific slot type to buffer the tuples, choose
the AM specific memory and flushing decisions etc.). Another advantage
with these new multi insert API is that the CREATE MATERIALIZED VIEW,
REFRESH MATERIALIZED VIEW, CREATE TABLE AS commands for heap AM got
faster by 62.54%, 68.87%, 74.31% or 2.67, 3.21, 3.89 times
respectively. The performance improvement in REFRESH MATERIALIZED VIEW
can benefit customers running analytical workloads on postgres.

I'm fine if we gradually add more infrastructure to support COPY,
INSERT INTO SELECT, Logical Replication Apply, Table Rewrites in
future releases. I'm sure it requires a lot more thoughts and time.

> is hard, but perhaps we can at least have an example one to demonstrate

The heap AM implements this new API. Also, there's a default
implementation for the new API falling back on to single inserts.
Aren't these sufficient to help AM implementers to come up with their
own implementations?

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 3 Apr 2024 17:55:27 +0530

> default APIs manually, but I'll see if I can add some tests to it the

Fixed a compiler warning found via CF bot. Please find the attached
v18 patches. I'm sorry for the noise.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 24 Apr 2024 18:19:40 +0530

> those that are implemented in your patch.

I added table_modify_begin, table_modify_buffer_insert,
table_modify_buffer_flush and table_modify_end. Table Access Method (AM)
authors now can define their own buffering strategy and flushing decisions
based on their tuple storage kinds and various other AM specific factors. I
also added a default implementation that falls back to single inserts when
no implementation is provided for these AM by AM authors. See the attached
v19-0001 patch.

> tuples are being flushed to get the right visibility semantics for

I added a flush callback named TableModifyBufferFlushCallback; when
provided by callers invoked after tuples are flushed to disk from the
buffers but before the AM frees them up. Index insertions and AFTER ROW
INSERT triggers can be executed in this callback. See the v19-0001 patch
for how AM invokes the flush callback, and see either v19-0003 or v19-0004
or v19-0005 for how a caller can supply the callback and required context
to execute index insertions and AR triggers.

> these can be initialized lazily to avoid overhead when not being used).

I haven't thought about these things yet. I can only focus on them after
seeing how the attached patches go from here.

> Perhaps just reporting memory usage and allowing the caller to force

Heap implementation for thes new Table AMs uses a separate memory context
for all of the operations. Please have a look and let me know if we need
anything more.

> straightforward, I believe, and handled by your patch. No indexes or

I used multi inserts for all of these including TABLE REWRITE commands such
as ALTER TABLE. See the attached v19-0002 patch. Check the testing section
below for benefits.

FWIW, following are some of the TABLE REWRITE commands that can get
benefitted:

ALTER TABLE tbl ALTER c1 TYPE bigint;
ALTER TABLE itest13 ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY;
ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
ALTER TABLE itest3 ALTER COLUMN a TYPE int;
ALTER TABLE gtest20 ALTER COLUMN b SET EXPRESSION AS (a * 3);
ALTER TABLE has_volatile ADD col4 int DEFAULT (random() * 10000)::int;
and so on.

> that buffering code path needs to be preserved in copyfrom.c even if

I modified the COPY FROM code to use the new Table AMs, and performed some
tests which show no signs of regression. Check the testing section below
for more details. See the attached v19-0005 patch. With this,
table_multi_insert can be deprecated.

> completion, so we need to be sure that the flush isn't forgotten in

I did that in v19-0003. I did place the table_modify_end call in multiple
places including ExecEndModifyTable. I didn't find any issues with it.
Please have a look and let me know if we need the end call in more places.
Check the testing section below for benefits.

> 10. Use these new methods for logical apply.

I used multi inserts for Logical Replication apply. in v19-0004. Check the
testing section below for benefits.

FWIW, open-source pglogical does have multi insert support, check code
around
https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_apply_heap.c#L960
.

> 11. Deprecate the multi_insert API.

I did remove both table_multi_insert and table_finish_bulk_insert in
v19-0006. Perhaps, removing them isn't a great idea, but adding a
deprecation WARNING/ERROR until some more PG releases might be worth
looking at.

> stone, or should we try to make all of these API changes together in

If the design, code and benefits that these new Table AMs bring to the
table look good, I hope to see it for PG 18.

> is hard, but perhaps we can at least have an example one to demonstrate

The attached patches already have implemented these new Table AMs for Heap.
I don't think we need a separate implementation to demonstrate. If others
feel so, I'm open to thoughts here.

Having said above, I'd like to reiterate the motivation behind the new
Table AMs for multi and single inserts.

1. A scan-like API with state being carried across is thought to be better
as suggested by Andres Freund -
https://www.postgresql.org/message-id/[email-masked]
.
2. Allowing a Table AM to optimize operations across multiple inserts,
define its own buffering strategy and take its own flushing decisions based
on their tuple storage kinds and various other AM specific factors.
3. Improve performance of various SQL commands with multi inserts for Heap
AM.

The attached v19 patches might need some more detailed comments, some
documentation and some specific tests ensuring the multi inserts for Heap
are kicked-in for various commands. I'm open to thoughts here.

I did some testing to see how various commands benefit with multi inserts
using these new Table AM for heap. It's not only the improvement in
performance these commands see, but also the amount of WAL that gets
generated reduces greatly. After all, multi inserts optimize the insertions
by writing less WAL. IOW, writing WAL record per page if multiple rows fit
into a single data page as opposed to WAL record per row.

Test case 1: 100 million rows, 2 columns (int and float)

Command                        | HEAD (sec) | PATCHED (sec) | Faster by % |
Faster by X
------------------------------ | ---------- | ------------- | ----------- |
-----------
CREATE TABLE AS                | 121        | 77            | 36.3        |
1.57
CREATE MATERIALIZED VIEW       | 101        | 49            | 51.4        |
2.06
REFRESH MATERIALIZED VIEW      | 113        | 54            | 52.2        |
2.09
ALTER TABLE (TABLE REWRITE)    | 124        | 81            | 34.6        |
1.53
COPY FROM                      | 71         | 72            | 0           |
1
INSERT INTO ... SELECT         | 117        | 62            | 47          |
1.88
LOGICAL REPLICATION APPLY      | 393        | 306           | 22.1        |
1.28

Command                        | HEAD (WAL in GB) | PATCHED (WAL in GB) |
Reduced by % | Reduced by X
------------------------------ | ---------------- | ------------------- |
------------ | -----------
CREATE TABLE AS                | 6.8              | 2.4                 |
64.7         | 2.83
CREATE MATERIALIZED VIEW       | 7.2              | 2.3                 |
68           | 3.13
REFRESH MATERIALIZED VIEW      | 10               | 5.1                 |
49           | 1.96
ALTER TABLE (TABLE REWRITE)    | 8                | 3.2                 |
60           | 2.5
COPY FROM                      | 2.9              | 3                   | 0
           | 1
INSERT INTO ... SELECT         | 8                | 3                   |
62.5         | 2.66
LOGICAL REPLICATION APPLY      | 7.5              | 2.3                 |
69.3         | 3.26

Test case 2: 1 billion rows, 1 column (int)

Command                        | HEAD (sec) | PATCHED (sec) | Faster by % |
Faster by X
------------------------------ | ---------- | ------------- | ----------- |
-----------
CREATE TABLE AS                | 794        | 386           | 51.38       |
2.05
CREATE MATERIALIZED VIEW       | 1006       | 563           | 44.03       |
1.78
REFRESH MATERIALIZED VIEW      | 977        | 603           | 38.28       |
1.62
ALTER TABLE (TABLE REWRITE)    | 1189       | 714           | 39.94       |
1.66
COPY FROM                      | 321        | 330           | -0.02       |
0.97
INSERT INTO ... SELECT         | 1084       | 586           | 45.94       |
1.84
LOGICAL REPLICATION APPLY      | 3530       | 2982          | 15.52       |
1.18

Command                        | HEAD (WAL in GB) | PATCHED (WAL in GB) |
Reduced by % | Reduced by X
------------------------------ | ---------------- | ------------------- |
------------ | -----------
CREATE TABLE AS                | 60               | 12                  |
80           | 5
CREATE MATERIALIZED VIEW       | 60               | 12                  |
80           | 5
REFRESH MATERIALIZED VIEW      | 60               | 12                  |
80           | 5
ALTER TABLE (TABLE REWRITE)    | 123              | 31                  |
60           | 2.5
COPY FROM                      | 12               | 12                  | 0
           | 1
INSERT INTO ... SELECT         | 120              | 24                  |
80           | 5
LOGICAL REPLICATION APPLY      | 61               | 12                  |
80.32        | 5

Test setup:
./configure --prefix=$PWD/pg17/ --enable-tap-tests CFLAGS="-ggdb3 -O2" >
install.log && make -j 8 install > install.log 2>&1 &

wal_level=logical
max_wal_size = 256GB
checkpoint_timeout = 1h

Test system is EC2 instance of type c5.4xlarge:
Architecture:            x86_64
  CPU op-mode(s):        32-bit, 64-bit
  Address sizes:         46 bits physical, 48 bits virtual
  Byte Order:            Little Endian
CPU(s):                  16
  On-line CPU(s) list:   0-15
Vendor ID:               GenuineIntel
  Model name:            Intel(R) Xeon(R) Platinum 8275CL CPU @ 3.00GHz
    CPU family:          6
    Model:               85
    Thread(s) per core:  2
    Core(s) per socket:  8
    Socket(s):           1
    Stepping:            7
    BogoMIPS:            5999.99
Caches (sum of all):
  L1d:                   256 KiB (8 instances)
  L1i:                   256 KiB (8 instances)
  L2:                    8 MiB (8 instances)
  L3:                    35.8 MiB (1 instance)
NUMA:
  NUMA node(s):          1
  NUMA node0 CPU(s):     0-15
RAM:
  MemTotal:       32036536 kB

---

From: Pavel Stehule <[email-masked]>
Date: Wed, 24 Apr 2024 18:07:03 +0200

st 24. 4. 2024 v 14:50 odesílatel Bharath Rupireddy <
[email-masked]> napsal:

> I used multi inserts for all of these including TABLE REWRITE commands
> such as ALTER TABLE. See the attached v19-0002 patch. Check the testing
> section below for benefits.
>
> The attached patches already have implemented these new Table AMs for
> Heap. I don't think we need a separate implementation to demonstrate. If
> others feel so, I'm open to thoughts here.
>
> Command                        | HEAD (sec) | PATCHED (sec) | Faster by %
> | Faster by X
> ------------------------------ | ---------- | ------------- | -----------
> | -----------
> CREATE TABLE AS                | 121        | 77            | 36.3
>  | 1.57
> CREATE MATERIALIZED VIEW       | 101        | 49            | 51.4
>  | 2.06
> REFRESH MATERIALIZED VIEW      | 113        | 54            | 52.2
>  | 2.09
> ALTER TABLE (TABLE REWRITE)    | 124        | 81            | 34.6
>  | 1.53
> COPY FROM                      | 71         | 72            | 0
> | 1
> INSERT INTO ... SELECT         | 117        | 62            | 47
>  | 1.88
> LOGICAL REPLICATION APPLY      | 393        | 306           | 22.1
>  | 1.28
>
> COPY FROM                      | 2.9              | 3                   |
> 0            | 1
> Command                        | HEAD (sec) | PATCHED (sec) | Faster by %
> | Faster by X
> ------------------------------ | ---------- | ------------- | -----------
> | -----------
> CREATE TABLE AS                | 794        | 386           | 51.38
> | 2.05
> CREATE MATERIALIZED VIEW       | 1006       | 563           | 44.03
> | 1.78
> REFRESH MATERIALIZED VIEW      | 977        | 603           | 38.28
> | 1.62
> ALTER TABLE (TABLE REWRITE)    | 1189       | 714           | 39.94
> | 1.66
> COPY FROM                      | 321        | 330           | -0.02
> | 0.97
> INSERT INTO ... SELECT         | 1084       | 586           | 45.94
> | 1.84
> LOGICAL REPLICATION APPLY      | 3530       | 2982          | 15.52
> | 1.18
>
> COPY FROM                      | 12               | 12                  |
> 0            | 1

looks pretty impressive!

Pavel

---

From: Jeff Davis <[email-masked]>
Date: Thu, 25 Apr 2024 09:41:08 -0700

On Wed, 2024-04-24 at 18:19 +0530, Bharath Rupireddy wrote:
> buffers but before the AM frees them up. Index insertions and AFTER
> ROW INSERT triggers can be executed in this callback. See the v19-
> 0001 patch for how AM invokes the flush callback, and see either v19-
> 0003 or v19-0004 or v19-0005 for how a caller can supply the callback
> and required context to execute index insertions and AR triggers.

The flush callback takes a pointer to an array of slot pointers, and I
don't think that's the right API. I think the callback should be called
on each slot individually.

We shouldn't assume that a table AM stores buffered inserts as an array
of slot pointers. A TupleTableSlot has a fair amount of memory overhead
(64 bytes), so most AMs wouldn't want to pay that overhead for every
tuple. COPY does, but that's because the number of buffered tuples is
fairly small.

> v19-0006.

That's OK with me. Let's leave those functions out for now.

> 
> If the design, code and benefits that these new Table AMs bring to
> the table look good, I hope to see it for PG 18.

Sounds good.

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 29 Apr 2024 11:36:20 +0530

> tuple. COPY does, but that's because the number of buffered tuples is

I get your point. An AM can choose to implement the buffering strategy
by just storing the plain tuple rather than the tuple slots in which
case the flush callback with an array of tuple slots won't work.
Therefore, I now changed the flush callback to accept only a single
tuple slot.

> That's OK with me. Let's leave those functions out for now.

Okay. Dropped the 0006 patch for now.

Please see the attached v20 patch set.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 15 May 2024 12:56:17 +0530

> Please see the attached v20 patch set.

It looks like with the use of the new multi insert table access method
(TAM) for COPY (v20-0005), pgbench regressed about 35% [1]. The reason
is that the memory-based flushing decision the new TAM takes [2]
differs from that of what the COPY does today with table_multi_insert.
The COPY with table_multi_insert, maintains exact size of the tuples
in CopyFromState after it does the line parsing. For instance, the
tuple size of a table with two integer columns is 8 (4+4) bytes here.
The new TAM relies on the memory occupied by the slot's memory context
which holds the actual tuple as a good approximation for the tuple
size. But, this memory context size also includes a tuple header, so
the size here is not just 8 (4+4) bytes but more. Because of this, the
buffers get flushed sooner than that of the existing COPY with
table_multi_insert AM causing regression in pgbench which uses COPY
extensively. The new TAM aren't designed to be able to receive tuple
sizes from the callers, even if we do that, the API doesn't look
generic.

Here are couple of ideas to get away with this:

1. Try to get the actual tuple sizes excluding header sizes for each
column in the new TAM.
2. Try not to use the new TAM for COPY in which case the
table_multi_insert stays forever.
3. Try passing a flag to tell the new TAM that the caller does the
flushing and no need for an internal flushing.

I haven't explored the idea (1) in depth yet. If we find a way to do
so, it looks to me that we are going backwards since we need to strip
off headers for each column of a row for all of the rows. I suspect
this would cost a bit more and may not solve the regression.

With an eventual goal to get rid of table_multi_insert, (3) may not be
a better choice.

(3) seems reasonable to implement and reduce the regression. I did so
in the attached v21 patches. A new flag TM_SKIP_INTERNAL_BUFFER_FLUSH
is introduced in v21 patch, when specified, no internal flushing is
done, the caller has to flush the buffered tuples using
table_modify_buffer_flush(). Check the test results [3] HEAD 2.948 s,
PATCHED 2.946 s.

v21 also adds code to maintain tuple size for virtual tuple slots.
This helps make better memory-based flushing decisions in the new TAM.

Thoughts?

[1]
HEAD:
done in 2.84 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 1.99 s, vacuum 0.21 s, primary keys 0.62 s).
done in 2.78 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 1.88 s, vacuum 0.21 s, primary keys 0.69 s).
done in 2.97 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.07 s, vacuum 0.21 s, primary keys 0.69 s).
done in 2.86 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 1.96 s, vacuum 0.21 s, primary keys 0.69 s).
done in 2.90 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.05 s, vacuum 0.21 s, primary keys 0.64 s).
done in 2.83 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 1.96 s, vacuum 0.21 s, primary keys 0.66 s).
done in 2.80 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 1.95 s, vacuum 0.20 s, primary keys 0.63 s).
done in 2.79 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 1.89 s, vacuum 0.21 s, primary keys 0.69 s).
done in 3.75 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.17 s, vacuum 0.32 s, primary keys 1.25 s).
done in 3.86 s (drop tables 0.00 s, create tables 0.08 s, client-side
generate 2.97 s, vacuum 0.21 s, primary keys 0.59 s).

AVG done in 2.948 s

v20 PATCHED:
done in 3.94 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.12 s, vacuum 0.19 s, primary keys 0.62 s).
done in 4.04 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.22 s, vacuum 0.20 s, primary keys 0.61 s).
done in 3.98 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.16 s, vacuum 0.20 s, primary keys 0.61 s).
done in 4.04 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.16 s, vacuum 0.20 s, primary keys 0.67 s).
done in 3.98 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.16 s, vacuum 0.20 s, primary keys 0.61 s).
done in 4.00 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.17 s, vacuum 0.20 s, primary keys 0.63 s).
done in 4.43 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.24 s, vacuum 0.21 s, primary keys 0.98 s).
done in 4.16 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 3.36 s, vacuum 0.20 s, primary keys 0.59 s).
done in 3.62 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.83 s, vacuum 0.20 s, primary keys 0.58 s).
done in 3.67 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.84 s, vacuum 0.21 s, primary keys 0.61 s).

AVG done in 3.986 s

[2]
+    /*
+     * Memory allocated for the whole tuple is in slot's memory context, so
+     * use it keep track of the total space occupied by all buffered tuples.
+     */
+    if (TTS_SHOULDFREE(slot))
+        mistate->cur_size += MemoryContextMemAllocated(slot->tts_mcxt, false);
+
+    if (mistate->cur_slots >= HEAP_MAX_BUFFERED_SLOTS ||
+        mistate->cur_size >= HEAP_MAX_BUFFERED_BYTES)
+        heap_modify_buffer_flush(state);

[3]
v21 PATCHED:
done in 2.92 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.12 s, vacuum 0.21 s, primary keys 0.59 s).
done in 2.89 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.07 s, vacuum 0.21 s, primary keys 0.61 s).
done in 2.89 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.05 s, vacuum 0.21 s, primary keys 0.62 s).
done in 2.90 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.07 s, vacuum 0.21 s, primary keys 0.62 s).
done in 2.80 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.00 s, vacuum 0.21 s, primary keys 0.59 s).
done in 2.84 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.04 s, vacuum 0.20 s, primary keys 0.60 s).
done in 2.84 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.03 s, vacuum 0.20 s, primary keys 0.59 s).
done in 2.85 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.04 s, vacuum 0.20 s, primary keys 0.60 s).
done in 3.48 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.44 s, vacuum 0.23 s, primary keys 0.80 s).
done in 3.05 s (drop tables 0.00 s, create tables 0.01 s, client-side
generate 2.28 s, vacuum 0.21 s, primary keys 0.55 s).

AVG done in 2.946 s

---

From: Jeff Davis <[email-masked]>
Date: Wed, 15 May 2024 16:31:42 -0700

On Wed, 2024-05-15 at 12:56 +0530, Bharath Rupireddy wrote:
> Because of this, the
> extensively.

The flushing behavior is entirely controlled by the table AM. The heap
can use the same flushing logic that it did before, which is to hold
1000 tuples.

I like that it's accounting for memory, too, but it doesn't need to be
overly restrictive. Why not just use work_mem? That should hold 1000
reasonably-sized tuples, plus overhead.

Even better would be if we could take into account partitioning. That
might be out of scope for your current work, but it would be very
useful. We could have a couple new GUCs like modify_table_buffer and
modify_table_buffer_per_partition or something like that.

> 1. Try to get the actual tuple sizes excluding header sizes for each

I don't see the point in arbitrarily excluding the header.

> This helps make better memory-based flushing decisions in the new

That seems wrong. We shouldn't need to change the TupleTableSlot
structure for this patch.

Comments on v21:

* All callers specify TM_FLAG_MULTI_INSERTS. What's the purpose?

* The only caller that doesn't use TM_FLAG_BAS_BULKWRITE is
ExecInsert(). What's the disadvantage to using a bulk insert state
there?

* I'm a bit confused by TableModifyState->modify_end_callback. The AM
both sets the callback and calls the callback -- why can't the code
just go into the table_modify_end method?

* The code structure in table_modify_begin() (and related) is strange.
Can it be simplified or am I missing something?

* Why are table_modify_state and insert_modify_buffer_flush_context
globals? What if there are multiple modify nodes in a plan?

* Can you explain the design in logical rep?

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Thu, 16 May 2024 12:00:36 -0700

> modify_table_buffer_per_partition or something like that.

To expand on this point:

For heap, the insert buffer is only 1000 tuples, which doesn't take
much memory. But for an AM that does any significant reorganization of
the input data, the buffer may be much larger. For insert into a
partitioned table, that buffer could be multiplied across many
partitions, and start to be a real concern.

We might not need table AM API changes at all here beyond what v21
offers. The ModifyTableState includes the memory context, so that gives
the caller a way to know the memory consumption of a single partition's
buffer. And if it needs to free the resources, it can just call
modify_table_end(), and then _begin() again if more tuples hit that
partition.

So I believe what I'm asking for here is entirely orthogonal to the
current proposal.

However, it got me thinking that we might not want to use work_mem for
controlling the heap's buffer size. Each AM is going to have radically
different memory needs, and may have its own (extension) GUCs to
control that memory usage, so they won't honor work_mem. We could
either have a separate GUC for the heap if it makes sense, or we could
just hard-code a reasonable value.

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 5 Jun 2024 12:42:17 +0530

Hi,

> modify_table_buffer_per_partition or something like that.

I disagree with inventing more GUCs. Instead, I'd vote for just
holding 1000 tuples in buffers for heap AM. This not only keeps the
code and new table AM simple, but also does not cause regression for
COPY. In my testing, 1000 tuples with 1 int and 1 float columns took
40000 bytes of memory (40 bytes each tuple), whereas with 1 int, 1
float and 1 text columns took 172000 bytes of memory (172 bytes each
tuple) bytes which IMO mustn't be a big problem. Thoughts?

> structure for this patch.

I dropped these ideas as I went ahead with the above idea of just
holding 1000 tuples in buffers for heap AM.

> * All callers specify TM_FLAG_MULTI_INSERTS. What's the purpose?

Previously, the multi insert state was initialized in modify_begin, so
it was then required to differentiate the code path. But, it's not
needed anymore with the lazy initialization of the multi insert state
moved to modify_buffer_insert. I removed it.

> ExecInsert(). What's the disadvantage to using a bulk insert state

The subsequent read queries will not find the just-now-inserted tuples
in shared buffers as a separate ring buffer is used with bulk insert
access strategy. The multi inserts is nothing but buffering multiple
tuples plus inserting in bulk. So using the bulk insert strategy might
be worth it for INSERT INTO SELECTs too. Thoughts?

> just go into the table_modify_end method?

I came up with modify_end_callback as per the discussion upthread to
use modify_begin, modify_end in future for UPDATE, DELETE and MERGE,
and not use any operation specific flags to clean the state
appropriately. The operation specific state cleaning logic can go to
the modify_end_callback implementation defined by the AM.

> Can it be simplified or am I missing something?

I previously defined these new table AMs as optional, check
GetTableAmRoutine(). And, there was a point upthread to provide
default/fallback implementation to help not fail insert operations on
tables without the new table AMs implemented. FWIW, the default
implementation was just doing the single inserts. The
table_modify_begin and friends need the logic to fallback making the
code there look different than other AMs. However, I now have a
feeling to drop the idea of having fallback implementation and let the
AMs deal with it. Although it might create some friction with various
non-core AM implementations, it keeps this patch simple which I would
vote for. Thoughts?

> globals? What if there are multiple modify nodes in a plan?

Can you please provide the case that can generate multiple "modify
nodes" in a single plan? AFAICS, multiple "modify nodes" in a plan can
exist for both partitioned tables and tables that get created as part
of CTEs. I disabled multi inserts for both of these cases. The way I
disabled for CTEs looks pretty naive - I just did the following. Any
better suggestions here to deal with all such cases?

+        if (operation == CMD_INSERT &&
+            nodeTag(subplanstate) == T_SeqScanState)
+            canMultiInsert = true;

> * Can you explain the design in logical rep?

Multi inserts for logical replication work at the table level. In
other words, all tuple inserts related to a single table within a
transaction are buffered and written to the corresponding table when
necessary. Whenever inserts pertaining to another table arrive, the
buffered tuples related to the previous table are written to the table
before starting the buffering for the new table. Also, the tuples are
written to the table from the buffer when there arrives a non-INSERT
operation, for example, UPDATE/DELETE/TRUNCATE/COMMIT etc. FWIW,
pglogical has the similar multi inserts logic -
https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_apply_heap.c#L879.

Please find the v22 patches with the above changes.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 26 Aug 2024 11:09:28 +0530

> Please find the v22 patches with the above changes.

Please find the v23 patches after rebasing 0005 and adapting 0004 for
9758174e2e.

---

From: Jeff Davis <[email-masked]>
Date: Mon, 26 Aug 2024 14:18:21 -0700

> Please find the v23 patches after rebasing 0005 and adapting 0004 for

Thank you.

0001 API design:

* Remove TableModifyState.modify_end_callback.

* This patch means that we will either remove or deprecate
TableAmRoutine.multi_insert and finish_bulk_insert. Are there any
strong opinions about maintaining support for multi-insert, or should
we just remove it outright and force any new AMs to implement the new
APIs to maintain COPY performance?

* Why do we need a separate "modify_flags" and "options"? Can't we just
combine them into TABLE_MODIFY_* flags?

Alexander, you had some work in this area as well, such b1484a3f19. I
believe 0001 covers this use case in a different way: rather than
giving complete responsibility to the AM to insert into the indexes,
the caller provides a callback and the AM is responsible for calling it
at the time the tuples are flushed. Is that right?

The design has been out for a while, so unless others have suggestions,
I'm considering the major design points mostly settled and I will move
forward with something like 0001 (pending implementation issues).

Note: I believe this API will extend naturally to updates and deletes,
as well.

0001 implementation issues:

* We need default implementations for AMs that don't implement the new
APIs, so that the AM will still function even if it only defines the
single-tuple APIs. If we need to make use of the AM's multi_insert
method (I'm not sure we do), then the default methods would need to
handle that as well. (I thought a previous version had these default
implementations -- is there a reason they were removed?)

* I am confused about how the heap implementation manages state and
resets it. mistate->mem_cxt is initialized to a new memory context in
heap_modify_begin, and then re-initialized to another new memory
context in heap_modify_buffer_insert. Then the mistate->mem_cxt is also
used as a temp context for executing heap_multi_insert, and it gets
reset before calling the flush callback, which still needs the slots.

* Why materialize the slot at copyfrom.c:1308 if the slot is going to
be copied anyway (which also materializes it; see
tts_virtual_copyslot()) at heapam.c:2710?

* After correcting the memory issues, can you get updated performance
numbers for COPY?

Regards,
	Jeff Davis

---

From: Matthias van de Meent <[email-masked]>
Date: Mon, 26 Aug 2024 23:59:28 +0200

> APIs to maintain COPY performance?

I don't think there is a significant enough difference in the
capabilities and requirements between the two APIs as currently
designed that removal of the old API would mean a significant
difference in capabilities. Maybe we could supply an equivalent API
shim to help the transition, but I don't think we should keep the old
API around in the TableAM.

> forward with something like 0001 (pending implementation issues).

Sorry about this late feedback, but while I'm generally +1 on the idea
and primary design, I feel that it doesn't quite cover all the areas
I'd expected it to cover.

Specifically, I'm having trouble seeing how this could be used to
implement ```INSERT INTO ... SELECT ... RETURNING ctid``` as I see no
returning output path for the newly inserted tuples' data, which is
usually required for our execution nodes' output path. Is support for
RETURN-clauses planned for this API? In a previous iteration, the
flush operation was capable of returning a TTS, but that seems to have
been dropped, and I can't quite figure out why.

> Note: I believe this API will extend naturally to updates and deletes,

I have the same concern about UPDATE ... RETURNING not fitting with
this callback-based design.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

---

From: Jeff Davis <[email-masked]>
Date: Mon, 26 Aug 2024 22:42:27 -0700

> been dropped, and I can't quite figure out why.

I'm not sure where that was lost, but I suspect when we changed
flushing to use a callback. I didn't get to v23-0003 yet, but I think
you're right that the current flushing mechanism isn't right for
returning tuples. Thank you.

One solution: when the buffer is flushed, we can return an iterator
over the buffered tuples to the caller. The caller can then use the
iterator to insert into indexes, return a tuple to the executor, etc.,
and then release the iterator when done (freeing the buffer). That
control flow is less convenient for most callers, though, so perhaps
that should be optional?

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Tue, 27 Aug 2024 14:37:27 -0700

> Please find the v23 patches after rebasing 0005 and adapting 0004 for

In patches 0002-0004, they must avoid the multi insert path when there
are before-row triggers, instead-of-row triggers, or volatile functions
used (see copyfrom.c:917-1006).

Also, until we decide on the RETURNING clause, we should block the
multi-insert path for that, as well, or implement it by using the
callback to copy tuples into the caller's context.

In 0003, why do you need the global insert_modify_buffer_flush_context?

0004 is the only place that calls table_modify_buffer_flush(). Is that
really necessary, or is automatic flushing enough?

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Tue, 27 Aug 2024 14:43:59 -0700

> implementations -- is there a reason they were removed?)

On second thought, it would be easier to just have the caller check
whether the AM supports the multi-insert path; and if not, fall back to
the single-tuple path. The single-tuple path is needed anyway for cases
like before-row triggers.

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 29 Aug 2024 12:55:59 +0530

> like before-row triggers.

Up until v21, the default implementation existed, see
https://www.postgresql.org/message-id/CALj2ACX90L5Mb5Vv%3DjsvhOdZ8BVsfpZf-CdCGhtm2N%2BbGUCSjg%40mail.gmail.com.
I then removed it in v22 to keep the code simple.

IMO, every caller branching out in the code like if (rel->rd_tableam->
tuple_modify_buffer_insert != NULL) then multi insert; else single
insert; doesn't look good. IMO, the default implementation approach
keeps things simple which eventually can be removed in *near* future.
Thoughts?

One change in the default implementation I would do from that of v21
is to assign the default AMs in GetTableAmRoutine() itself to avoid if
.. else if .. else in the table_modify_XXX().

---

From: Jeff Davis <[email-masked]>
Date: Thu, 29 Aug 2024 12:29:43 -0700

On Thu, 2024-08-29 at 12:55 +0530, Bharath Rupireddy wrote:
> IMO, every caller branching out in the code like if (rel->rd_tableam-
> >

I believe we need the branching in the caller anyway:

1. If there is a BEFORE row trigger with a volatile function, the
visibility rules[1] mean that the function should see changes from all
the rows inserted so far this command, which won't work if they are
still in the buffer.

2. Similarly, for an INSTEAD OF row trigger, the visibility rules say
that the function should see all previous rows inserted.

3. If there are volatile functions in the target list or WHERE clause,
the same visibility semantics apply.

4. If there's a "RETURNING ctid" clause, we need to either come up with
a way to return the tuples after flushing, or we need to use the
single-tuple path. (Similarly in the future when we support UPDATE ...
RETURNING, as Matthias pointed out.)

If we need two paths in each caller anyway, it seems cleaner to just
wrap the check for tuple_modify_buffer_insert in
table_modify_buffer_enabled().

We could perhaps use a one path and then force a batch size of one or
something, which is an alternative, but we have to be careful not to
introduce a regression (and it still requires a solution for #4).

Regards,
	Jeff Davis

[1] 
https://www.postgresql.org/docs/devel/trigger-datachanges.html

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 26 Oct 2024 06:30:50 -0700

Hi,

> introduce a regression (and it still requires a solution for #4).

I chose to branch in the caller e.g. if there's a volatile function
SELECT query of REFRESH MATERIALIZED VIEW, the caller goes
table_tuple_insert() path, else multi-insert path.

I am posting the new v24 patch set organized as follows: 0001
introducing the new table AM, 0002 optimizing CTAS, CMV and RMV, 0003
using the new table AM for COPY ... FROM. I, for now, discarded the
INSERT INTO ... SELECT and Logical Replication Apply patches, the idea
is to take the basic stuff forward.

I reworked structure names, members and function names, reworded
comments, addressed review comments in the v24 patches. Please have a
look.

---

From: Jingtang Zhang <[email-masked]>
Date: Mon, 28 Oct 2024 16:43:09 +0800

Hi! Glad to see update in this thread.

Little question about v24 0002 patch: would it be better to move the
implementation of TableModifyIsMultiInsertsSupported to somewhere for table
AM
level? Seems it is a common function for future use, not a specific one for
matview.

---

Regards, Jingtang

Bharath Rupireddy <[email-masked]> 于2024年10月26日周六

---

From: Jingtang Zhang <[email-masked]>
Date: Mon, 28 Oct 2024 22:48:34 +0800

Oh, another comments for v24-0001 patch: we are in heam AM now, should we use
something like HEAP_INSERT_BAS_BULKWRITE instead of using table AM option,
just like other heap AM options do?

> +		if ((state->options & TABLE_INSERT_BAS_BULKWRITE) != 0)
> +			istate->bistate = GetBulkInsertState();

—

Regards, Jingtang