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

On Fri, Jun 7, 2019 at 12:51 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > 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.

That's fair enough to a point, but I'm not trying to enforce code
reuse; I'm trying to make it possible. If it's good enough for the
heap, which is really the gold standard for AMs until somebody manages
to do better, it's entirely reasonable for somebody else to want to
just do it the way the heap does. We gain nothing by making that
difficult.

> > 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:

Hmm, so what we would we do here? Just 'struct BulkInsertState;
typedef struct BulkInsertState BulkInsertState;' ... and then never
actually define the struct anywhere?

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

OK, that's an option. I guess we'd change free_bulk_insert to take
the BulkInsertState as an additional option?

> 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?

Nope.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company