Re: should INSERT SELECT use a BulkInsertState?

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(at)telsasoft(dot)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