Hi, On 2019-05-14 16:27:47 -0400, Alvaro Herrera wrote: > On 2019-May-14, Ashwin Agrawal wrote: > > > Thank you. Please find the patch to rename the agreed functions. It would > > be good to make all consistent instead of applying exception to three > > functions but seems no consensus on it. Given table_ api are new, we could > > modify them leaving systable_ ones as is, but as objections left that as is. > > Hmm .. I'm surprised to find out that we only have one caller of > simple_table_insert, simple_table_delete, simple_table_update. I'm not > sure I agree to the new names those got in the renaming patch, since > they're not really part of table AM proper ... do we really want to > offer those as separate entry points? Why not just remove those routines?
I don't think it'd be better if execReplication.c has them inline - we'd just have the exact same code inline. There's plenty extension out there that use simple_heap_*, and without such wrappers, they'll all have to copy the contents of simple_table_* too. Also we'll probably want to switch CatalogTuple* over to them at some point. > Somewhat related: it's strange that CatalogTupleUpdate etc use > simple_heap_update instead of the tableam variants wrappers (I suppose > that's either because of bootstrapping concerns, or performance). It's because the callers currently expect to work with heap tuples, rather than slots. And changing that would have been a *LOT* of work (as in: prohibitively much for v12). I didn't want to create a slot for each insertion, because that'd make them slower. But as Robert said on IM (discussing something else), we already create a slot in most cases, via CatalogIndexInsert(). Not sure if HOT updates and deletes are common enough to make the slot creation in those cases measurable. > Would it be too strange to have CatalogTupleInsert call heap_insert() > directly, and do away with simple_heap_insert? (Equivalently for > update, delete). I think those wrappers made perfect sense when we had > simple_heap_insert all around the place ... but now that we introduced > the CatalogTupleFoo wrappers, I don't think it does any longer. I don't really see the advantage. Won't that just break a lot of code that will continue to work otherwise, as long as you just use heap tables? With the sole benefit of moving a bit of code from one place to another? Greetings, Andres Freund