should INSERT SELECT use a BulkInsertState?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

should INSERT SELECT use a BulkInsertState?

Justin Pryzby
Seems to me it should, at least conditionally.  At least if there's a function
scan or a relation or ..

I mentioned a bit about our use-case here:
https://www.postgresql.org/message-id/20200219173742.GA30939%40telsasoft.com
=> I'd prefer our loaders to write their own data rather than dirtying large
fractions of buffer cache and leaving it around for other backends to clean up.

commit 7f9e061363e58f30eee0cccc8a0e46f637bf137b
Author: Justin Pryzby <[hidden email]>
Date:   Fri May 8 02:17:32 2020 -0500

    Make INSERT SELECT use a BulkInsertState

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 20a4c474cc..6da4325225 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -578,7 +578,7 @@ ExecInsert(ModifyTableState *mtstate,
  table_tuple_insert_speculative(resultRelationDesc, slot,
    estate->es_output_cid,
    0,
-   NULL,
+   mtstate->bistate,
    specToken);
 
  /* insert index entries for tuple */
@@ -617,7 +617,7 @@ ExecInsert(ModifyTableState *mtstate,
  /* insert the tuple normally */
  table_tuple_insert(resultRelationDesc, slot,
    estate->es_output_cid,
-   0, NULL);
+   0, mtstate->bistate);
 
  /* insert index entries for tuple */
  if (resultRelInfo->ri_NumIndices > 0)
@@ -2332,6 +2332,14 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
 
  mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans);
  mtstate->mt_nplans = nplans;
+ mtstate->bistate = NULL;
+ if (operation == CMD_INSERT)
+ {
+ Plan *p = linitial(node->plans);
+ Assert(nplans == 1);
+ if (!IsA(p, Result) && !IsA(p, ValuesScan))
+ mtstate->bistate = GetBulkInsertState();
+ }
 
  /* set up epqstate with dummy subplan data for the moment */
  EvalPlanQualInit(&mtstate->mt_epqstate, estate, NULL, NIL, node->epqParam);
@@ -2809,6 +2817,9 @@ ExecEndModifyTable(ModifyTableState *node)
  */
  for (i = 0; i < node->mt_nplans; i++)
  ExecEndNode(node->mt_plans[i]);
+
+ if (node->bistate)
+ FreeBulkInsertState(node->bistate);
 }
 
 void
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 4fee043bb2..daf365f181 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -14,6 +14,7 @@
 #ifndef EXECNODES_H
 #define EXECNODES_H
 
+#include "access/heapam.h"
 #include "access/tupconvert.h"
 #include "executor/instrument.h"
 #include "fmgr.h"
@@ -1177,6 +1178,7 @@ typedef struct ModifyTableState
  List  **mt_arowmarks; /* per-subplan ExecAuxRowMark lists */
  EPQState mt_epqstate; /* for evaluating EvalPlanQual rechecks */
  bool fireBSTriggers; /* do we need to fire stmt triggers? */
+ BulkInsertState bistate; /* State for bulk insert like INSERT SELECT */
 
  /*
  * Slot for storing tuples in the root partitioned table's rowtype during


Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Justin Pryzby
On Fri, May 08, 2020 at 02:25:45AM -0500, Justin Pryzby wrote:
> Seems to me it should, at least conditionally.  At least if there's a function
> scan or a relation or ..
>
> I mentioned a bit about our use-case here:
> https://www.postgresql.org/message-id/20200219173742.GA30939%40telsasoft.com
> => I'd prefer our loaders to write their own data rather than dirtying large
> fractions of buffer cache and leaving it around for other backends to clean up.

Nobody suggested otherwise so I added here and cleaned up to pass tests.
https://commitfest.postgresql.org/28/2553/

--
Justin

v1-0001-Make-INSERT-SELECT-use-a-BulkInsertState.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Michael Paquier-2
In reply to this post by Justin Pryzby
On Fri, May 08, 2020 at 02:25:45AM -0500, Justin Pryzby wrote:
> Seems to me it should, at least conditionally.  At least if there's a function
> scan or a relation or ..
>
> I mentioned a bit about our use-case here:
> https://www.postgresql.org/message-id/20200219173742.GA30939%40telsasoft.com
> => I'd prefer our loaders to write their own data rather than dirtying large
> fractions of buffer cache and leaving it around for other backends to clean up.

Does it matter in terms of performance and for which cases does it
actually matter?

> diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
> index 4fee043bb2..daf365f181 100644
> --- a/src/include/nodes/execnodes.h
> +++ b/src/include/nodes/execnodes.h
> @@ -14,6 +14,7 @@
>  #ifndef EXECNODES_H
>  #define EXECNODES_H
>  
> +#include "access/heapam.h"
>  #include "access/tupconvert.h"
>  #include "executor/instrument.h"
>  #include "fmgr.h"
> @@ -1177,6 +1178,7 @@ typedef struct ModifyTableState
>   List  **mt_arowmarks; /* per-subplan ExecAuxRowMark lists */
>   EPQState mt_epqstate; /* for evaluating EvalPlanQual rechecks */
>   bool fireBSTriggers; /* do we need to fire stmt triggers? */
> + BulkInsertState bistate; /* State for bulk insert like INSERT SELECT */
I think that this needs more thoughts.  You are introducing a
dependency between some generic execution-related nodes and heap, a
table AM.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Andres Freund
In reply to this post by Justin Pryzby
Hi,

On 2020-05-08 02:25:45 -0500, Justin Pryzby wrote:
> Seems to me it should, at least conditionally.  At least if there's a function
> scan or a relation or ..

Well, the problem is that this can cause very very significant
regressions. As in 10x slower or more. The ringbuffer can cause constant
XLogFlush() calls (due to the lsn interlock), and the eviction from
shared_buffers (regardless of actual available) will mean future vacuums
etc will be much slower.  I think this is likely to cause pretty
widespread regressions on upgrades.

Now, it sucks that we have this problem in the general facility that's
supposed to be used for this kind of bulk operation. But I don't really
see it realistic as expanding use of bulk insert strategies unless we
have some more fundamental fixes.

Regards,

Andres


Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Daniel Gustafsson
> On 4 Jun 2020, at 19:30, Andres Freund <[hidden email]> wrote:
> On 2020-05-08 02:25:45 -0500, Justin Pryzby wrote:

>> Seems to me it should, at least conditionally.  At least if there's a function
>> scan or a relation or ..
>
> Well, the problem is that this can cause very very significant
> regressions. As in 10x slower or more. The ringbuffer can cause constant
> XLogFlush() calls (due to the lsn interlock), and the eviction from
> shared_buffers (regardless of actual available) will mean future vacuums
> etc will be much slower.  I think this is likely to cause pretty
> widespread regressions on upgrades.
>
> Now, it sucks that we have this problem in the general facility that's
> supposed to be used for this kind of bulk operation. But I don't really
> see it realistic as expanding use of bulk insert strategies unless we
> have some more fundamental fixes.

Based on the above, and the lack of activity in the thread, it sounds like this
patch should be marked Returned with Feedback; but Justin: you set it to
Waiting on Author at the start of the commitfest, are you working on a new
version?

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Justin Pryzby
In reply to this post by Andres Freund
On Thu, Jun 04, 2020 at 10:30:47AM -0700, Andres Freund wrote:

> On 2020-05-08 02:25:45 -0500, Justin Pryzby wrote:
> > Seems to me it should, at least conditionally.  At least if there's a function
> > scan or a relation or ..
>
> Well, the problem is that this can cause very very significant
> regressions. As in 10x slower or more. The ringbuffer can cause constant
> XLogFlush() calls (due to the lsn interlock), and the eviction from
> shared_buffers (regardless of actual available) will mean future vacuums
> etc will be much slower.  I think this is likely to cause pretty
> widespread regressions on upgrades.
>
> Now, it sucks that we have this problem in the general facility that's
> supposed to be used for this kind of bulk operation. But I don't really
> see it realistic as expanding use of bulk insert strategies unless we
> have some more fundamental fixes.
I made this conditional on BEGIN BULK/SET bulk, so I'll solicit comments on that.

postgres=# \t on \\ \set QUIET \\ VACUUM FULL t; \dt+ t \\ begin ; \timing on \\ INSERT INTO t SELECT * FROM t; rollback; SELECT COUNT(1), usagecount FROM pg_buffercache GROUP BY 2 ORDER BY 2;
| public | t    | table | pryzbyj | 35 MB |
|Time: 9497.318 ms (00:09.497)
|    33 |          1
|     3 |          2
|    18 |          3
|     5 |          4
|  4655 |          5
| 11670 |          

vs

postgres=# \t on \\ \set QUIET \\ VACUUM FULL t; \dt+ t \\ begin BULK ; \timing on \\ INSERT INTO t SELECT * FROM t; rollback; SELECT COUNT(1), usagecount FROM pg_buffercache GROUP BY 2 ORDER BY 2;
| public | t    | table | pryzbyj | 35 MB |
|Time: 8268.780 ms (00:08.269)
|  2080 |          1
|     3 |          2
|    19 |          4
|   234 |          5
| 14048 |          

And:

postgres=# begin ; \x \\ \t \\ SELECT statement_timestamp(); \o /dev/null \\ SELECT 'INSERT INTO t VALUES(0)' FROM generate_series(1,999999); \set ECHO errors \\ \set QUIET on \\ \o \\ \gexec \\ SELECT statement_timestamp(); abort; \x \\ SELECT COUNT(1), usagecount FROM pg_buffercache GROUP BY 2 ORDER BY 2; a
|statement_timestamp | 2020-07-12 20:31:43.717328-05
|statement_timestamp | 2020-07-12 20:36:16.692469-05
|
|    52 |          1
|    24 |          2
|    17 |          3
|     6 |          4
|  4531 |          5
| 11754 |          

vs

postgres=# begin BULK ; \x \\ \t \\ SELECT statement_timestamp(); \o /dev/null \\ SELECT 'INSERT INTO t VALUES(0)' FROM generate_series(1,999999); \set ECHO errors \\ \set QUIET on \\ \o \\ \gexec \\ SELECT statement_timestamp(); abort; \x \\ SELECT COUNT(1), usagecount FROM pg_buffercache GROUP BY 2 ORDER BY 2; a
|statement_timestamp | 2020-07-12 20:43:47.089538-05
|statement_timestamp | 2020-07-12 20:48:04.798138-05
|
|  4456 |          1
|    22 |          2
|     1 |          3
|     7 |          4
|    79 |          5
| 11819 |

--
Justin

v2-0001-Allow-INSERT-SELECT-to-use-a-BulkInsertState.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Justin Pryzby
On Sun, Jul 12, 2020 at 08:57:00PM -0500, Justin Pryzby wrote:

> On Thu, Jun 04, 2020 at 10:30:47AM -0700, Andres Freund wrote:
> > On 2020-05-08 02:25:45 -0500, Justin Pryzby wrote:
> > > Seems to me it should, at least conditionally.  At least if there's a function
> > > scan or a relation or ..
> >
> > Well, the problem is that this can cause very very significant
> > regressions. As in 10x slower or more. The ringbuffer can cause constant
> > XLogFlush() calls (due to the lsn interlock), and the eviction from
> > shared_buffers (regardless of actual available) will mean future vacuums
> > etc will be much slower.  I think this is likely to cause pretty
> > widespread regressions on upgrades.
> >
> > Now, it sucks that we have this problem in the general facility that's
> > supposed to be used for this kind of bulk operation. But I don't really
> > see it realistic as expanding use of bulk insert strategies unless we
> > have some more fundamental fixes.
>
> I made this conditional on BEGIN BULK/SET bulk, so I'll solicit comments on that.
@cfbot: rebased

v3-0001-Allow-INSERT-SELECT-to-use-a-BulkInsertState.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Justin Pryzby
On Sat, Sep 19, 2020 at 08:32:15AM -0500, Justin Pryzby wrote:

> On Sun, Jul 12, 2020 at 08:57:00PM -0500, Justin Pryzby wrote:
> > On Thu, Jun 04, 2020 at 10:30:47AM -0700, Andres Freund wrote:
> > > On 2020-05-08 02:25:45 -0500, Justin Pryzby wrote:
> > > > Seems to me it should, at least conditionally.  At least if there's a function
> > > > scan or a relation or ..
> > >
> > > Well, the problem is that this can cause very very significant
> > > regressions. As in 10x slower or more. The ringbuffer can cause constant
> > > XLogFlush() calls (due to the lsn interlock), and the eviction from
> > > shared_buffers (regardless of actual available) will mean future vacuums
> > > etc will be much slower.  I think this is likely to cause pretty
> > > widespread regressions on upgrades.
> > >
> > > Now, it sucks that we have this problem in the general facility that's
> > > supposed to be used for this kind of bulk operation. But I don't really
> > > see it realistic as expanding use of bulk insert strategies unless we
> > > have some more fundamental fixes.
> >
> > I made this conditional on BEGIN BULK/SET bulk, so I'll solicit comments on that.
>
> @cfbot: rebased
again

--
Justin

v4-0001-Allow-INSERT-SELECT-to-use-a-BulkInsertState.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Simon Riggs
In reply to this post by Andres Freund
On Thu, 4 Jun 2020 at 18:31, Andres Freund <[hidden email]> wrote:

> On 2020-05-08 02:25:45 -0500, Justin Pryzby wrote:
> > Seems to me it should, at least conditionally.  At least if there's a function
> > scan or a relation or ..
>
> Well, the problem is that this can cause very very significant
> regressions. As in 10x slower or more. The ringbuffer can cause constant
> XLogFlush() calls (due to the lsn interlock), and the eviction from
> shared_buffers (regardless of actual available) will mean future vacuums
> etc will be much slower.  I think this is likely to cause pretty
> widespread regressions on upgrades.
>
> Now, it sucks that we have this problem in the general facility that's
> supposed to be used for this kind of bulk operation. But I don't really
> see it realistic as expanding use of bulk insert strategies unless we
> have some more fundamental fixes.

Are you saying that *anything* that uses the BulkInsertState is
generally broken? We use it for VACUUM and COPY writes, so you are
saying they are broken??

When we put that in, the use of the ringbuffer for writes required a
much larger number of blocks to smooth out the extra XLogFlush()
calls, but overall it was a clear win in those earlier tests. Perhaps
the ring buffer needs to be increased, or made configurable. The
eviction behavior was/is deliberate, to avoid large data loads
spoiling cache - perhaps that could also be configurable for the case
where data fits in shared buffers.

Anyway, if we can discuss what you see as broken, we can fix that and
then extend the usage to other cases, such as INSERT SELECT.

--
Simon Riggs                http://www.EnterpriseDB.com/


Reply | Threaded
Open this post in threaded view
|

Re: should INSERT SELECT use a BulkInsertState?

Simon Riggs
In reply to this post by Justin Pryzby
On Fri, 16 Oct 2020 at 22:05, Justin Pryzby <[hidden email]> wrote:

> > > I made this conditional on BEGIN BULK/SET bulk, so I'll solicit comments on that.

I think it would be better if this was self-tuning. So that we don't
allocate a bulkinsert state until we've done say 100 (?) rows
inserted.

If there are other conditions under which this is non-optimal
(Andres?), we can also autodetect that and avoid them.

You should also use table_multi_insert() since that will give further
performance gains by reducing block access overheads. Switching from
single row to multi-row should also only happen once we've loaded a
few rows, so we don't introduce overahads for smaller SQL statements.

--
Simon Riggs                http://www.EnterpriseDB.com/