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

Hi,

(David, see bottom if you're otherwise not interested).

On 2019-06-07 09:48:29 -0400, Robert Haas wrote:
> On Sat, Jun 1, 2019 at 3:20 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > I'd like to think that the best way to deal with that and reduce the
> > > confusion would be to move anything related to bulk inserts into their
> > > own header/file, meaning the following set:
> > > - ReleaseBulkInsertStatePin
> > > - GetBulkInsertState
> > > - FreeBulkInsertState
> > > There is the argument that we could also move that part into tableam.h
> > > itself though as some of the rather generic table-related callbacks,
> > > but that seems grotty. So I think that we could just move that stuff
> > > as backend/access/common/bulkinsert.c.
> >
> > Yea, I think we should do that at some point. But I'm not sure this is
> > the right design. Bulk insert probably needs to rather be something
> > that's allocated inside the AM.
>
> As far as I can see, any on-disk, row-oriented, block-based AM is
> likely to want the same implementation as the heap.

I'm pretty doubtful about that. It'd e.g. would make a ton of sense to
keep the VM pinned, even for heap. You could also do a lot better with
toast. And for zheap we'd - unless we change the design - quite
possibly benefit from keeping the last needed tpd buffer around.

> Here's a draft design for adding some abstraction, roughly modeled on
> the abstraction Andres added for TupleTableSlots:

Hm, I'm not sure I see the need for a vtable based approach here. Won't
every AM know exactly what they need / have? I'm not convinced it's
worthwhile to treat that separately from the tableam. I.e. have a
BulkInsertState struct with *no* members, and then, as you suggest:

>
> 3. table AM gets a new member BulkInsertState
> *(*create_bistate)(Relation Rel) and a corresponding function
> table_create_bistate(), analogous to table_create_slot(), which can
> call the constructor function for the appropriate type of
> BulkInsertState and return the result

but also route the following through the AM:

> 2. that structure has a member for each defined operation on a BulkInsertState:
>
> void (*free)(BulkInsertState *);
> void (*release_pin)(BulkInsertState *); // maybe rename to make it more generic

Where free would just be part of finish_bulk_insert, and release_pin a
new callback.

> 4. each type of BulkInsertState has its own functions for making use
> of it, akin to ReadBufferBI.

Right, I don't think that's avoidable unfortunately.

> I see Michael's point about the relationship between
> finish_bulk_insert() and the BulkInsertState, and maybe if we could
> figure that out we could avoid the need for a BulkInsertState to have
> a free method (or maybe any methods at all, in which case it could
> just be an opaque struct, like a Node).

Right, so we actually eneded up at the same place. And you found a bug:

> 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()).

Greetings,

Andres Freund