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

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