pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Justin Pryzby <pryzby@telsasoft.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: should INSERT SELECT use a BulkInsertState?
Date: Fri, 16 Oct 2020 16:05:12 -0500
Message-ID: <20201016210511.GC9241@telsasoft.com> (raw )
In-Reply-To: <20200919133215.GB30557@telsasoft.com >
References: <20200508072545.GA9701@telsasoft.com >
<20200604173047.dirdp6zln7ogj54k@alap3.anarazel.de >
<20200713015700.GA23581@telsasoft.com >
<20200919133215.GB30557@telsasoft.com >
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
Attachments:
v4-0001-Allow-INSERT-SELECT-to-use-a-BulkInsertState.patch (text/x-diff, 7.1K) download
From 7f856d0597a34a98be848c337612f1671497f52f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 8 May 2020 02:17:32 -0500
Subject: [PATCH v4] Allow INSERT SELECT to use a BulkInsertState
---
src/backend/executor/nodeModifyTable.c | 22 ++++++++++++++++++++--
src/backend/parser/gram.y | 7 ++++++-
src/backend/tcop/utility.c | 4 ++++
src/backend/utils/misc/guc.c | 11 +++++++++++
src/include/executor/nodeModifyTable.h | 2 ++
src/include/nodes/execnodes.h | 2 ++
src/include/parser/kwlist.h | 1 +
7 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 0c055ed408..d19d6d1a85 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -76,6 +76,8 @@ static ResultRelInfo *getTargetResultRelInfo(ModifyTableState *node);
static void ExecSetupChildParentMapForSubplan(ModifyTableState *mtstate);
static TupleConversionMap *tupconv_map_for_subplan(ModifyTableState *node,
int whichplan);
+/* guc */
+bool insert_in_bulk = false;
/*
* Verify that the tuples to be produced by INSERT or UPDATE match the
@@ -598,7 +600,7 @@ ExecInsert(ModifyTableState *mtstate,
table_tuple_insert_speculative(resultRelationDesc, slot,
estate->es_output_cid,
0,
- NULL,
+ NULL, /* Bulk insert not supported */
specToken);
/* insert index entries for tuple */
@@ -638,7 +640,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)
@@ -2365,6 +2367,16 @@ 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 &&
+ node->onConflictAction != ONCONFLICT_UPDATE &&
+ node->rootRelation == 0)
+ {
+ Assert(nplans == 1);
+
+ if (insert_in_bulk)
+ mtstate->bistate = GetBulkInsertState();
+ }
/* set up epqstate with dummy subplan data for the moment */
EvalPlanQualInit(&mtstate->mt_epqstate, estate, NULL, NIL, node->epqParam);
@@ -2805,6 +2817,12 @@ ExecEndModifyTable(ModifyTableState *node)
resultRelInfo);
}
+ if (node->bistate)
+ {
+ FreeBulkInsertState(node->bistate);
+ table_finish_bulk_insert((getTargetResultRelInfo(node))->ri_RelationDesc, 0);
+ }
+
/*
* Close all the partitioned tables, leaf partitions, and their indices
* and release the slot used for tuple routing, if set.
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 480d168346..b87e31e36a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -631,7 +631,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
ASSERTION ASSIGNMENT ASYMMETRIC AT ATTACH ATTRIBUTE AUTHORIZATION
BACKWARD BEFORE BEGIN_P BETWEEN BIGINT BINARY BIT
- BOOLEAN_P BOTH BY
+ BOOLEAN_P BOTH BY BULK
CACHE CALL CALLED CASCADE CASCADED CASE CAST CATALOG_P CHAIN CHAR_P
CHARACTER CHARACTERISTICS CHECK CHECKPOINT CLASS CLOSE
@@ -9873,6 +9873,9 @@ transaction_mode_item:
| NOT DEFERRABLE
{ $$ = makeDefElem("transaction_deferrable",
makeIntConst(false, @1), @1); }
+ | BULK
+ { $$ = makeDefElem("bulk",
+ makeIntConst(true, @1), @1); }
;
/* Syntax with commas is SQL-spec, without commas is Postgres historical */
@@ -15079,6 +15082,7 @@ unreserved_keyword:
| BACKWARD
| BEFORE
| BEGIN_P
+ | BULK
| BY
| CACHE
| CALL
@@ -15590,6 +15594,7 @@ bare_label_keyword:
| BIT
| BOOLEAN_P
| BOTH
+ | BULK
| BY
| CACHE
| CALL
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 9a35147b26..cb3933b0e9 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -611,6 +611,10 @@ standard_ProcessUtility(PlannedStmt *pstmt,
SetPGVariable("transaction_deferrable",
list_make1(item->arg),
true);
+ else if (strcmp(item->defname, "bulk") == 0)
+ SetPGVariable("bulk_insert",
+ list_make1(item->arg),
+ true);
}
}
break;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a62d64eaa4..5f2fc9004b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -54,6 +54,7 @@
#include "libpq/pqformat.h"
#include "miscadmin.h"
#include "optimizer/cost.h"
+#include "executor/nodeModifyTable.h"
#include "optimizer/geqo.h"
#include "optimizer/optimizer.h"
#include "optimizer/paths.h"
@@ -2036,6 +2037,16 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"bulk_insert", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the transaction to bulk insert mode."),
+ gettext_noop("A ring buffer of limited size will be used."),
+ },
+ &insert_in_bulk,
+ false,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
diff --git a/src/include/executor/nodeModifyTable.h b/src/include/executor/nodeModifyTable.h
index 46a2dc9511..09c312a052 100644
--- a/src/include/executor/nodeModifyTable.h
+++ b/src/include/executor/nodeModifyTable.h
@@ -15,6 +15,8 @@
#include "nodes/execnodes.h"
+extern PGDLLIMPORT bool insert_in_bulk;
+
extern void ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
EState *estate, TupleTableSlot *slot,
CmdType cmdtype);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index b7e9e5d539..50af9dabf5 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"
@@ -1159,6 +1160,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
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 71dcdf2889..0991da11e7 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -60,6 +60,7 @@ PG_KEYWORD("binary", BINARY, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
PG_KEYWORD("bit", BIT, COL_NAME_KEYWORD, BARE_LABEL)
PG_KEYWORD("boolean", BOOLEAN_P, COL_NAME_KEYWORD, BARE_LABEL)
PG_KEYWORD("both", BOTH, RESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("bulk", BULK, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("by", BY, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("cache", CACHE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("call", CALL, UNRESERVED_KEYWORD, BARE_LABEL)
--
2.17.0
view thread (27+ messages) latest in thread
Message-ID: <20201016210511.GC9241@telsasoft.com>
Permalink: ../20201016210511.GC9241@telsasoft.com/
Also on: postgresql.org/message-id/20201016210511.GC9241@telsasoft.com
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, andres@anarazel.de, pgsql-hackers@lists.postgresql.org
Subject: Re: should INSERT SELECT use a BulkInsertState?
In-Reply-To: <20201016210511.GC9241@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox