Re: Custom table AMs need to include heapam.h because of BulkInsertState

From: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Custom table AMs need to include heapam.h because of BulkInsertState
Date: 2019-06-09 23:45:17
Message-ID: CAKJS1f_0t-K0_3xe+erXPQ-jgaOb6tRZayErCXF2RpGdUVMt9g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sat, 8 Jun 2019 at 04:51, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2019-06-07 09:48:29 -0400, Robert Haas wrote:
> > However, it looks to me as though copy.c can create a bunch of
> > BulkInsertStates but only call finish_bulk_insert() once, so unless
> > that's a bug in need of fixing I don't quite see how to make that
> > approach work.
>
> That is a bug. Not a currently "active" one with in-core AMs (no
> dangerous bulk insert flags ever get set for partitioned tables), but we
> obviously need to fix it nevertheless.
>
> Robert, seems we'll have to - regardless of where we come down on fixing
> this bug - have to make copy use multiple BulkInsertState's, even in the
> CIM_SINGLE (with proute) case. Or do you have a better idea?
>
> David, any opinions on how to best fix this? It's not extremely obvious
> how to do so best in the current setup of the partition actually being
> hidden somewhere a few calls away (i.e. the table_open happening in
> ExecInitPartitionInfo()).

That's been overlooked. I agree it's not a bug with heap, since
heapam_finish_bulk_insert() only does anything there when we're
skipping WAL, which we don't do in copy.c for partitioned tables.
However, who knows what other AMs will need, so we'd better fix that.

My proposed patch is attached.

I ended up moving the call to CopyMultiInsertInfoCleanup() down to
after we call table_finish_bulk_insert for the main table. This might
not be required but I lack imagination right now to what AMs might put
in the finish_bulk_insert call, so doing this seems safer.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachment Content-Type Size
fix_copy_table_finish_build_insert.patch application/octet-stream 2.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-06-10 00:33:10 Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
Previous Message Andres Freund 2019-06-09 21:31:26 Re: Temp table handling after anti-wraparound shutdown (Was: BUG #15840)