should INSERT SELECT use a BulkInsertState?

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:
/messages/by-id/20200219173742.GA30939@telsasoft.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

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:
/messages/by-id/20200219173742.GA30939@telsasoft.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

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

On 4 Jun 2020, at 19:30, Andres Freund <andres@anarazel.de> 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

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

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

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

On Thu, 4 Jun 2020 at 18:31, Andres Freund <andres@anarazel.de> 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/

On Fri, 16 Oct 2020 at 22:05, Justin Pryzby <pryzby@telsasoft.com> 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/

On Thu, Oct 22, 2020 at 01:29:53PM +0100, Simon Riggs wrote:

On Fri, 16 Oct 2020 at 22:05, Justin Pryzby <pryzby@telsasoft.com> 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.

I made it an optional, non-default behavior in response to the legitimate
concern for performance regression for the cases where a loader needs to be as
fast as possible - as compared with our case, where we want instead to optimize
for our reports by making the loaders responsible for their own writes, rather
than leaving behind many dirty pages, and clobbering the cache, too.

Also, INSERT SELECT doesn't immediately help us (telsasoft), since we use
INSERT .. VALUES () .. ON CONFLICT. This would handle that case, which is
great, even though that wasn't a design goal. It could also be an integer GUC
to allow configuring the size of the ring buffer.

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.

Good idea...multi_insert (which reduces the overhead of individual inserts) is
mostly independent from BulkInsert state (which uses a ring-buffer to avoid
dirtying the cache). I made this 0002.

This makes INSERT SELECT several times faster, and not clobber the cache too.

Time: 4700.606 ms (00:04.701)
123 | 1
37 | 2
20 | 3
11 | 4
4537 | 5
11656 |

Time: 1125.302 ms (00:01.125)
2171 | 1
37 | 2
20 | 3
11 | 4
111 | 5
14034 |

When enabled, this passes nearly all regression tests, and all but 2 of the
changes are easily understood. The 2nd patch still needs work.

--
Justin

On 30.10.20 05:51, Justin Pryzby wrote:

On Thu, Oct 22, 2020 at 01:29:53PM +0100, Simon Riggs wrote:

On Fri, 16 Oct 2020 at 22:05, Justin Pryzby <pryzby@telsasoft.com> 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.

I made it an optional, non-default behavior in response to the legitimate
concern for performance regression for the cases where a loader needs to be as
fast as possible - as compared with our case, where we want instead to optimize
for our reports by making the loaders responsible for their own writes, rather
than leaving behind many dirty pages, and clobbering the cache, too.

Also, INSERT SELECT doesn't immediately help us (telsasoft), since we use
INSERT .. VALUES () .. ON CONFLICT. This would handle that case, which is
great, even though that wasn't a design goal. It could also be an integer GUC
to allow configuring the size of the ring buffer.

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.

Good idea...multi_insert (which reduces the overhead of individual inserts) is
mostly independent from BulkInsert state (which uses a ring-buffer to avoid
dirtying the cache). I made this 0002.

This makes INSERT SELECT several times faster, and not clobber the cache too.

Time: 4700.606 ms (00:04.701)
123 | 1
37 | 2
20 | 3
11 | 4
4537 | 5
11656 |

Time: 1125.302 ms (00:01.125)
2171 | 1
37 | 2
20 | 3
11 | 4
111 | 5
14034 |

When enabled, this passes nearly all regression tests, and all but 2 of the
changes are easily understood. The 2nd patch still needs work.

Hi,

Came across this thread because I'm working on an improvement for the
relation extension to improve the speed of the bulkinsert itself in
(highly) parallel cases and would like to make sure that our approaches
work nicely together.

Given what I've seen and tried so far with various benchmarks I would
also really like to see a different approach here. The "BEGIN BULK" can
be problematic for example if you mix small amounts of inserts and big
amounts in the same transaction, or if your application possibly does a
bulk insert but otherwise mostly OLTP transactions.

To me the idea from Simon sounds good to only use a bulk insert state
after inserting e.g. a 1000 rows, and this also seems more applicable to
most applications compared to requiring a change to any application that
wishes to have faster ingest.

Another approach could be to combine this, for example, with a few extra
requirements to limit the amount of regressions and first learn more how
this behaves in the field.
We could, for example, only (just throwing out some ideas), require that:
- the relation has a certain size
- a BufferStrategy a maximum certain size is used
- there is a certain amount of lock waiters on relation extension. (like
we do with bulk extend)
- we have extended the relation for at least e.g. 4 MB and not used the
FSM anymore thereby proving that we are doing bulk operations instead of
random small extensions everywhere into the relation that use the FSM.

Another thing is that we first try to improve the bulk operation
facilities in general and then have another shot at this? Not sure if
there is some benchmark / query that shows where such a 10x slowdown
would appear but maybe that would be worth a look as well possibly.

Regards,
Luc

On Mon, Nov 02, 2020 at 07:53:45AM +0100, Luc Vlaming wrote:

On 30.10.20 05:51, Justin Pryzby wrote:

On Thu, Oct 22, 2020 at 01:29:53PM +0100, Simon Riggs wrote:

On Fri, 16 Oct 2020 at 22:05, Justin Pryzby <pryzby@telsasoft.com> 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.

I made it an optional, non-default behavior in response to the legitimate
concern for performance regression for the cases where a loader needs to be as
fast as possible - as compared with our case, where we want instead to optimize
for our reports by making the loaders responsible for their own writes, rather
than leaving behind many dirty pages, and clobbering the cache, too.

Also, INSERT SELECT doesn't immediately help us (telsasoft), since we use
INSERT .. VALUES () .. ON CONFLICT. This would handle that case, which is
great, even though that wasn't a design goal. It could also be an integer GUC
to allow configuring the size of the ring buffer.

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.

Good idea...multi_insert (which reduces the overhead of individual inserts) is
mostly independent from BulkInsert state (which uses a ring-buffer to avoid
dirtying the cache). I made this 0002.

This makes INSERT SELECT several times faster, and not clobber the cache too.

Time: 4700.606 ms (00:04.701)
123 | 1
37 | 2
20 | 3
11 | 4
4537 | 5
11656 |

Time: 1125.302 ms (00:01.125)
2171 | 1
37 | 2
20 | 3
11 | 4
111 | 5
14034 |

When enabled, this passes nearly all regression tests, and all but 2 of the
changes are easily understood. The 2nd patch still needs work.

Hi,

Came across this thread because I'm working on an improvement for the
relation extension to improve the speed of the bulkinsert itself in (highly)
parallel cases and would like to make sure that our approaches work nicely

Thanks for looking.

Since this is a GUC, I thought it would accomodate users optimizing for either
inserts vs selects, as well as users who don't want to change their application
(they can "ALTER SYSTEM SET bulk_insert=on"). I'm not thrilled about making a
new guc, but that seems to be required for "begin bulk", which was the obvious
way to make it an 'opt-in' feature.

I guess it'd be easy to add a counter to ModifyTableState, although it makes
the code a bit less clean and conceivably performs "discontinuously" - inserts
100rows/sec for the first 999 rows and then 200rows/sec afterwards.

If you "mix" small inserts and big inserts, it would be a bad strategy to
optimize for the small ones. Anyway, in a quick test, small inserts were not
slower.
/messages/by-id/20200713015700.GA23581@telsasoft.com

Do you have an example that regresses with bulk insert ?

The two patches are separate, and it's possible they should be enabled
differently or independently.

--
Justin

On Mon, Nov 02, 2020 at 12:45:51PM -0600, Justin Pryzby wrote:

On Mon, Nov 02, 2020 at 07:53:45AM +0100, Luc Vlaming wrote:

On 30.10.20 05:51, Justin Pryzby wrote:

On Thu, Oct 22, 2020 at 01:29:53PM +0100, Simon Riggs wrote:

On Fri, 16 Oct 2020 at 22:05, Justin Pryzby <pryzby@telsasoft.com> 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.

I made it an optional, non-default behavior in response to the legitimate
concern for performance regression for the cases where a loader needs to be as
fast as possible - as compared with our case, where we want instead to optimize
for our reports by making the loaders responsible for their own writes, rather
than leaving behind many dirty pages, and clobbering the cache, too.

Also, INSERT SELECT doesn't immediately help us (telsasoft), since we use
INSERT .. VALUES () .. ON CONFLICT. This would handle that case, which is
great, even though that wasn't a design goal. It could also be an integer GUC
to allow configuring the size of the ring buffer.

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.

Good idea...multi_insert (which reduces the overhead of individual inserts) is
mostly independent from BulkInsert state (which uses a ring-buffer to avoid
dirtying the cache). I made this 0002.

This makes INSERT SELECT several times faster, and not clobber the cache too.

- Rebased on Heikki's copy.c split;
- Rename structures without "Copy" prefix;
- Move MultiInsert* from copyfrom.c to (tentatively) nodeModifyTable.h;
- Move cur_lineno and transition_capture into MultiInsertInfo;

This switches to multi insert after a configurable number of tuples.
If set to -1, that provides the historic behavior that bulk inserts
can leave behind many dirty buffers. Perhaps that should be the default.

I guess this shouldn't be in copy.h or in commands/* at all.
It'll be included by both: commands/copyfrom_internal.h and
executor/nodeModifyTable.h. Maybe it should go in util or lib...
I don't know how to do it without including executor.h, which seems
to be undesirable.

--
Justin

On Mon, Nov 23, 2020 at 08:00:20PM -0600, Justin Pryzby wrote:

On Mon, Nov 02, 2020 at 12:45:51PM -0600, Justin Pryzby wrote:

On Mon, Nov 02, 2020 at 07:53:45AM +0100, Luc Vlaming wrote:

On 30.10.20 05:51, Justin Pryzby wrote:

On Thu, Oct 22, 2020 at 01:29:53PM +0100, Simon Riggs wrote:

On Fri, 16 Oct 2020 at 22:05, Justin Pryzby <pryzby@telsasoft.com> 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.

I made it an optional, non-default behavior in response to the legitimate
concern for performance regression for the cases where a loader needs to be as
fast as possible - as compared with our case, where we want instead to optimize
for our reports by making the loaders responsible for their own writes, rather
than leaving behind many dirty pages, and clobbering the cache, too.

Also, INSERT SELECT doesn't immediately help us (telsasoft), since we use
INSERT .. VALUES () .. ON CONFLICT. This would handle that case, which is
great, even though that wasn't a design goal. It could also be an integer GUC
to allow configuring the size of the ring buffer.

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.

Good idea...multi_insert (which reduces the overhead of individual inserts) is
mostly independent from BulkInsert state (which uses a ring-buffer to avoid
dirtying the cache). I made this 0002.

This makes INSERT SELECT several times faster, and not clobber the cache too.

- Rebased on Heikki's copy.c split;
- Rename structures without "Copy" prefix;
- Move MultiInsert* from copyfrom.c to (tentatively) nodeModifyTable.h;
- Move cur_lineno and transition_capture into MultiInsertInfo;

This switches to multi insert after a configurable number of tuples.
If set to -1, that provides the historic behavior that bulk inserts
can leave behind many dirty buffers. Perhaps that should be the default.

I guess this shouldn't be in copy.h or in commands/* at all.
It'll be included by both: commands/copyfrom_internal.h and
executor/nodeModifyTable.h. Maybe it should go in util or lib...
I don't know how to do it without including executor.h, which seems
to be undesirable.

Attached resolves issue with FDW contrib by including the MultiInsertInfo
structure rather than a pointer and makes the logic more closely match
copyfrom.c related to partition/triggers.

I had made this a conditional based on the concern that bulk insert state would
cause regression. But then it occurred to me that COPY uses a bulk insert
unconditionally. Should COPY be conditional, too ? Or maybe that's ok, since
COPY is assumed to be a bulk operation.

--
Justin

One loose end in this patch is how to check for volatile default expressions.

copyfrom.c is a utility statement, so it can look at the parser's column list:
COPY table(c1,c2)...

However, for INSERT, in nodeModifyTable.c, it looks like parsing, rewriting,
and planning are done, at which point I don't know if there's a good way to
find that. The default expressions will have been rewritten into the planned
statement.

We need the list of columns whose default is volatile, excluding columns for
which a non-default value is specified.

INSERT INTO table (c1,c2) VALUES (1,default);

We'd want the list of any column in the table with a volatile default,
excluding columns c1, but not-excluding explicit default columns c2 or any
implicit default columns (c3, etc).

Any idea ?

--
Justin

On Wed, Dec 2, 2020 at 10:24 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

One loose end in this patch is how to check for volatile default expressions.

copyfrom.c is a utility statement, so it can look at the parser's column list:
COPY table(c1,c2)...

However, for INSERT, in nodeModifyTable.c, it looks like parsing, rewriting,
and planning are done, at which point I don't know if there's a good way to
find that. The default expressions will have been rewritten into the planned
statement.

We need the list of columns whose default is volatile, excluding columns for
which a non-default value is specified.

INSERT INTO table (c1,c2) VALUES (1,default);

We'd want the list of any column in the table with a volatile default,
excluding columns c1, but not-excluding explicit default columns c2 or any
implicit default columns (c3, etc).

Any idea ?

I think we should be doing all the necessary checks in the planner and
have a flag in the planned stmt to indicate whether to go with multi
insert or not. For the required checks, we can have a look at how the
existing COPY decides to go with either CIM_MULTI or CIM_SINGLE.

Now, the question of how we can get to know whether a given relation
has default expressions or volatile expressions, it is worth to look
at build_column_default() and contain_volatile_functions().

I prefer to have the multi insert deciding code in COPY and INSERT
SELECT, in a single common function which can be reused. Though COPY
has somethings like default expressions and others ready unlike INSERT
SELECT, we can try to keep them under a common function and say for
COPY we can skip some code and for INSERT SELECT we can do extra work
to find default expressions.

Although unrelated, for parallel inserts in INSERT SELECT[1]/messages/by-id/CAJcOf-fy3P+kDArvmbEtdQTxFMf7Rn2=V-sqCnMmKO3QKBsgPA@mail.gmail.com, in the
planner there are some checks to see if the parallelism is safe or
not. Check max_parallel_hazard_for_modify() in
v8-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch from
[1]: /messages/by-id/CAJcOf-fy3P+kDArvmbEtdQTxFMf7Rn2=V-sqCnMmKO3QKBsgPA@mail.gmail.com
code.

[1]: /messages/by-id/CAJcOf-fy3P+kDArvmbEtdQTxFMf7Rn2=V-sqCnMmKO3QKBsgPA@mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Dec 03, 2020 at 10:59:34AM +0530, Bharath Rupireddy wrote:

On Wed, Dec 2, 2020 at 10:24 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

One loose end in this patch is how to check for volatile default expressions.

I think we should be doing all the necessary checks in the planner and
have a flag in the planned stmt to indicate whether to go with multi
insert or not. For the required checks, we can have a look at how the
existing COPY decides to go with either CIM_MULTI or CIM_SINGLE.

Yes, you can see that I've copied the checks from copy.
Like copy, some checks are done once, in ExecInitModifyTable, outside of the
ExecModifyTable "loop".

This squishes some commits together.
And uses bistate for ON CONFLICT.
And attempts to use memory context for tuple size.

For the bufferedBytes check, I'm not sure what's best. Copy flushes buffers
after 65k of input line length, but that's totally different from tuple slot
memory context size, which is what I used for insert. Maybe COPY should also
use slot size? Or maybe the threshold to flush needs to be set in miinfo,
rather than a #define, and differ between COPY and INSERT.

--
Justin

On Sat, Dec 05, 2020 at 01:59:41PM -0600, Justin Pryzby wrote:

On Thu, Dec 03, 2020 at 10:59:34AM +0530, Bharath Rupireddy wrote:

On Wed, Dec 2, 2020 at 10:24 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

One loose end in this patch is how to check for volatile default expressions.

I think we should be doing all the necessary checks in the planner and
have a flag in the planned stmt to indicate whether to go with multi
insert or not. For the required checks, we can have a look at how the
existing COPY decides to go with either CIM_MULTI or CIM_SINGLE.

Yes, you can see that I've copied the checks from copy.
Like copy, some checks are done once, in ExecInitModifyTable, outside of the
ExecModifyTable "loop".

This squishes some commits together.
And uses bistate for ON CONFLICT.
And attempts to use memory context for tuple size.

Rebased on 9dc718bdf2b1a574481a45624d42b674332e2903

I guess my patch should/may be subsumed by this other one - I'm fine with that.
https://commitfest.postgresql.org/31/2871/

Note that my interest here is just in bistate, to avoid leaving behind many
dirty buffers, not improved performance of COPY.

--
Justin

On Mon, Feb 22, 2021 at 02:25:22AM +0000, houzj.fnst@fujitsu.com wrote:

Yes, you can see that I've copied the checks from copy.
Like copy, some checks are done once, in ExecInitModifyTable, outside
of the ExecModifyTable "loop".

This squishes some commits together.
And uses bistate for ON CONFLICT.
And attempts to use memory context for tuple size.

Rebased on 9dc718bdf2b1a574481a45624d42b674332e2903

I guess my patch should/may be subsumed by this other one - I'm fine with
that.
https://commitfest.postgresql.org/31/2871/

Note that my interest here is just in bistate, to avoid leaving behind many dirty
buffers, not improved performance of COPY.

I am very interested in this patch, and I plan to do some experiments with the patch.
Can you please rebase the patch because it seems can not applied to the master now.

Thanks for your interest.

I was sitting on a rebased version since the bulk FDW patch will cause
conflicts, and since this should maybe be built on top of the table-am patch
(2871). Have fun :)

--
Justin