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

From: Andres Freund <[email-masked]>
Date: Sat, 3 Jun 2023 15:38:24 -0700

Hi,

This patch was referenced in a discussion at pgcon, so I thought I'd give it a
look, even though Bharat said that he won't have time to drive it forward...

On 2021-04-19 10:21:36 +0530, Bharath Rupireddy wrote:
> diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
> index bd5faf0c1f..655de8e6b7 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -2558,6 +2558,11 @@ static const TableAmRoutine heapam_methods = {
>  	.tuple_insert_speculative = heapam_tuple_insert_speculative,
>  	.tuple_complete_speculative = heapam_tuple_complete_speculative,
>  	.multi_insert = heap_multi_insert,
> +	.tuple_insert_begin = heap_insert_begin,
> +	.tuple_insert_v2 = heap_insert_v2,
> +	.multi_insert_v2 = heap_multi_insert_v2,
> +	.multi_insert_flush = heap_multi_insert_flush,
> +	.tuple_insert_end = heap_insert_end,
>  	.tuple_delete = heapam_tuple_delete,
>  	.tuple_update = heapam_tuple_update,
>  	.tuple_lock = heapam_tuple_lock,

I don't think we should have multiple callback for the insertion APIs in
tableam.h. I think it'd be good to continue supporting the old table_*()
functions, but supporting multiple insert APIs in each AM doesn't make much
sense to me.

> +/*
> + * GetTupleSize - Compute the tuple size given a table slot.
> + *
> + * For heap tuple, buffer tuple and minimal tuple slot types return the actual
> + * tuple size that exists. For virtual tuple, the size is calculated as the
> + * slot does not have the tuple size. If the computed size exceeds the given
> + * maxsize for the virtual tuple, this function exits, not investing time in
> + * further unnecessary calculation.
> + *
> + * Important Notes:
> + * 1) Size calculation code for virtual slots is being used from
> + * 	  tts_virtual_materialize(), hence ensure to have the same changes or fixes
> + * 	  here and also there.
> + * 2) Currently, GetTupleSize() handles the existing heap, buffer, minimal and
> + * 	  virtual slots. Ensure to add related code in case any new slot type is
> + *    introduced.
> + */
> +inline Size
> +GetTupleSize(TupleTableSlot *slot, Size maxsize)
> +{
> +	Size sz = 0;
> +	HeapTuple tuple = NULL;
> +
> +	if (TTS_IS_HEAPTUPLE(slot))
> +		tuple = ((HeapTupleTableSlot *) slot)->tuple;
> +	else if(TTS_IS_BUFFERTUPLE(slot))
> +		tuple = ((BufferHeapTupleTableSlot *) slot)->base.tuple;
> +	else if(TTS_IS_MINIMALTUPLE(slot))
> +		tuple = ((MinimalTupleTableSlot *) slot)->tuple;
> +	else if(TTS_IS_VIRTUAL(slot))

I think this embeds too much knowledge of the set of slot types in core
code. I don't see why it's needed either?

> diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
> index 414b6b4d57..2a1470a7b6 100644
> --- a/src/include/access/tableam.h
> +++ b/src/include/access/tableam.h
> @@ -229,6 +229,32 @@ typedef struct TM_IndexDeleteOp
>  	TM_IndexStatus *status;
>  } TM_IndexDeleteOp;
>
> +/* Holds table insert state. */
> +typedef struct TableInsertState

I suspect we should design it to be usable for updates and deletes in the
future, and thus name it TableModifyState.

> +{
> +	Relation	rel;
> +	/* Bulk insert state if requested, otherwise NULL. */
> +	struct BulkInsertStateData	*bistate;
> +	CommandId	cid;

Hm - I'm not sure it's a good idea to force the cid to be the same for all
inserts done via one TableInsertState.

> +	int	options;
> +	/* Below members are only used for multi inserts. */
> +	/* Array of buffered slots. */
> +	TupleTableSlot	**mi_slots;
> +	/* Number of slots that are currently buffered. */
> +	int32	mi_cur_slots;

> +	/*
> +	 * Access method specific information such as parameters that are needed
> +	 * for buffering and flushing decisions can go here.
> +	 */
> +	void	*mistate;

I think we should instead have a generic TableModifyState, which each AM then
embeds into an AM specific AM state. Forcing two very related structs to be
allocated separately doesn't seem wise in this case.

> @@ -1430,6 +1473,50 @@ table_multi_insert(Relation rel, TupleTableSlot **slots, int nslots,
>  								  cid, options, bistate);
>  }
>
> +static inline TableInsertState*
> +table_insert_begin(Relation rel, CommandId cid, int options,
> +				   bool alloc_bistate, bool is_multi)

Why have alloc_bistate and options?

> +static inline void
> +table_insert_end(TableInsertState *state)
> +{
> +	/* Deallocate bulk insert state here, since it's AM independent. */
> +	if (state->bistate)
> +		FreeBulkInsertState(state->bistate);
> +
> +	state->rel->rd_tableam->tuple_insert_end(state);
> +}

Seems like the order in here should be swapped?

Greetings,

Andres Freund

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 5 Jun 2023 08:00:00 +0530

> look, even though Bharat said that he won't have time to drive it forward...

Thanks. I'm glad to know that the feature was discussed at PGCon.

If there's an interest, I'm happy to spend time again on it.

I'll look into the review comments and respond soon.

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

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 1 Aug 2023 22:00:00 +0530

> look, even though Bharat said that he won't have time to drive it forward...

Thanks. Finally, I  started to spend time on this. Just curious - may
I know the discussion in/for which this patch is referenced? What was
the motive? Is it captured somewhere?

> functions, but supporting multiple insert APIs in each AM doesn't make much

I named these new functions XXX_v2 for compatibility reasons. Because,
it's quite possible for external modules to use existing
table_tuple_insert, table_multi_insert functions. If we were to change
the existing insert tableams, all the external modules using them
would have to change their code, is that okay?

> code. I don't see why it's needed either?

The heapam multi-insert implementation needs to know the tuple size
from the slot to decide whether or not to flush the tuples from the
buffers. I couldn't find a direct way then to know the tuple size from
the slot, so added that helper function. With a better understanding
now, I think we can rely on the memory allocated for TupleTableSlot's
tts_mcxt. While this works for the materialized slots passed in to the
insert functions, for non-materialized slots the flushing decision can
be solely on the number of tuples stored in the buffers. Another way
is to add a get_tuple_size callback to TupleTableSlotOps and let the
tuple slot providers give us the tuple size.

> future, and thus name it TableModifyState.

There are different parameters that insert/update/delete would want to
pass across in the state. So, having Table{Insert/Update/Delete}State
may be a better idea than having the unneeded variables lying around
or having a union and state_type as INSERT/UPDATE/DELETE, no? Do you
have a different thought here?

> allocated separately doesn't seem wise in this case.

The v7 patches have largely changed the way these options and
parameters are passed, please have a look.

> inserts done via one TableInsertState.

If required, someone can always pass a new CID before every
tuple_insert_v2/tuple_multi_insert_v2 call via TableInsertState. Isn't
it sufficient?

> Why have alloc_bistate and options?

"alloc_bistate" is for the caller to specify if they need a bulk
insert state or not. "options" is for the caller to specify if they
need table_tuple_insert performance options such as
TABLE_INSERT_SKIP_FSM, TABLE_INSERT_FROZEN, TABLE_INSERT_NO_LOGICAL.
The v7 patches have changed the way these options and parameters are
passed, please have a look.

> Seems like the order in here should be swapped?

Right. It looks like BulkInsertState is for heapam, it really doesn't
have to be in table_XXX functions, hence it all the way down to
heap_insert_XXX functions.

I'm attaching the v7 patch set with the above review comments
addressed. My initial idea behind these new insert APIs was the
ability to re-use the multi insert code in COPY for CTAS and REFRESH
MATERIALIZED VIEW. I'm open to more thoughts here.

The v7 patches have largely changed the way state structure (heapam
specific things are moved all the way down to heapam.c) is defined,
the parameters are passed, and simplified the multi insert logic a
lot.

0001 - introduces new single and multi insert table AM and heapam
implementation of the new AM.
0002 - optimizes CREATE TABLE AS to use the new multi inserts table AM
making it faster by 2.13X or 53%.
0003 - optimizes REFRESH MATERIALIZED VIEW to use the new multi
inserts table AM making it faster by 1.52X or 34%.
0004 - uses the new multi inserts table AM for COPY FROM - I'm yet to
spend time on this, I'll share the patch when ready.

Thoughts?

---

From: Jacob Champion <[email-masked]>
Date: Tue, 1 Aug 2023 10:02:12 -0700

> the motive? Is it captured somewhere?

It may not have been the only place, but we at least touched on it
during the unconference:

    https://wiki.postgresql.org/wiki/PgCon_2023_Developer_Unconference#Table_AMs

We discussed two related-but-separate ideas:
1) bulk/batch operations and
2) maintenance of TAM state across multiple related operations.

--Jacob

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 17 Jan 2024 22:57:48 +0530

> 2) maintenance of TAM state across multiple related operations.

Thank you. I'm attaching v8 patch-set here which includes use of new
insert TAMs for COPY FROM. With this, postgres not only will have the
new TAM for inserts, but they also can make the following commands
faster - CREATE TABLE AS, SELECT INTO, CREATE MATERIALIZED VIEW,
REFRESH MATERIALIZED VIEW and COPY FROM. I'll perform some testing in
the coming days and post the results here, until then I appreciate any
feedback on the patches.

I've also added this proposal to CF -
https://commitfest.postgresql.org/47/4777/.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 29 Jan 2024 12:57:00 +0530

> https://commitfest.postgresql.org/47/4777/.

Some of the tests related to Incremental Sort added by a recent commit
0452b461bc4 in aggregates.sql are failing when the multi inserts
feature is used for CTAS (like done in 0002 patch). I'm not so sure if
it's because of the reduction in the CTAS execution times. Execution
time for table 'btg' created with CREATE TABLE AS added by commit
0452b461bc4 with single inserts is 25.3 msec, with multi inserts is
17.7 msec. This means that the multi inserts are about 1.43 times or
30.04% faster  than the single inserts. Couple of ways to make these
tests pick Incremental Sort as expected - 1) CLUSTER btg USING abc; or
2) increase the number of rows in table btg to 100K from 10K. FWIW, if
I reduce the number of rows in the table from 10K to 1K, the
Incremental Sort won't get picked on HEAD with CTAS using single
inserts. Hence, I chose option (2) to fix the issue.

Please find the attached v9 patch set.

[1]
 -- Engage incremental sort
 explain (COSTS OFF) SELECT x,y FROM btg GROUP BY x,y,z,w;
-                   QUERY PLAN
--------------------------------------------------
+          QUERY PLAN
+------------------------------
  Group
    Group Key: x, y, z, w
-   ->  Incremental Sort
+   ->  Sort
          Sort Key: x, y, z, w
-         Presorted Key: x, y
-         ->  Index Scan using btg_x_y_idx on btg
-(6 rows)
+         ->  Seq Scan on btg
+(5 rows)

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 29 Jan 2024 17:16:47 +0530

>  explain (COSTS OFF) SELECT x,y FROM btg GROUP BY x,y,z,w;

CF bot machine with Windows isn't happy with the compilation [1], so
fixed those warnings and attached v10 patch set.

[1]
[07:35:25.458] [632/2212] Compiling C object
src/backend/postgres_lib.a.p/commands_copyfrom.c.obj
[07:35:25.458] c:\cirrus\src\include\access\tableam.h(1574) : warning
C4715: 'table_multi_insert_slots': not all control paths return a
value
[07:35:25.458] c:\cirrus\src\include\access\tableam.h(1522) : warning
C4715: 'table_insert_begin': not all control paths return a value
[07:35:25.680] c:\cirrus\src\include\access\tableam.h(1561) : warning
C4715: 'table_multi_insert_next_free_slot': not all control paths
return a value
[07:35:25.680] [633/2212] Compiling C object
src/backend/postgres_lib.a.p/commands_createas.c.obj
[07:35:25.680] c:\cirrus\src\include\access\tableam.h(1522) : warning
C4715: 'table_insert_begin': not all control paths return a value
[07:35:26.310] [646/2212] Compiling C object
src/backend/postgres_lib.a.p/commands_matview.c.obj
[07:35:26.310] c:\cirrus\src\include\access\tableam.h(1522) : warning
C4715: 'table_insert_begin': not all control paths return a value

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 2 Mar 2024 12:02:29 +0530

> > Please find the attached v9 patch set.

I've had to rebase the patches due to commit 874d817, please find the
attached v11 patch set.

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 8 Mar 2024 16:06:59 +0530

> I've had to rebase the patches due to commit 874d817, please find the

Rebase needed. Please see the v12 patch set.

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 19 Mar 2024 14:09:36 +0900

Hi,

> Rebase needed. Please see the v12 patch set.

I've not reviewed the patches in depth yet, but run performance tests
for CREATE MATERIALIZED VIEW. The test scenarios is:

-- setup
create unlogged table test (c int);
insert into test select generate_series(1, 10000000);

-- run
create materialized view test_mv as select * from test;

Here are the results:

* HEAD
3775.221 ms
3744.039 ms
3723.228 ms

* v12 patch
6289.972 ms
5880.674 ms
7663.509 ms

I can see performance regressions and the perf report says that CPU
spent most time on extending the ResourceOwner's array while copying
the buffer-heap tuple:

- 52.26% 0.18% postgres postgres [.] intorel_receive
    52.08% intorel_receive
        table_multi_insert_v2 (inlined)
        - heap_multi_insert_v2
            - 51.53% ExecCopySlot (inlined)
                tts_buffer_heap_copyslot
                tts_buffer_heap_store_tuple (inlined)
             - IncrBufferRefCount
                 - ResourceOwnerEnlarge
                     ResourceOwnerAddToHash (inlined)

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

Regards,

-- 
Masahiko Sawada