On Fri, Jun 7, 2019 at 12:51 PM Andres Freund <and...@anarazel.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
