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