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

On Mon, 10 Jun 2019 at 11:45, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>
> On Sat, 8 Jun 2019 at 04:51, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > 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.

Andres, do you want to look at this before I look again?

Do you see any issue with calling table_finish_bulk_insert() when the
partition's CopyMultiInsertBuffer is evicted from the
CopyMultiInsertInfo rather than at the end of the copy? It can mean
that we call the function multiple times per partition. I assume the
function is only really intended to flush bulk inserted tuple to the
storage, so calling it more than once would just mean an inefficiency
rather than a bug.

Let me know your thoughts.

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