Hi,
For the tableam work I'd like to remove heapam.h from
nodeModifyTable.c. The only remaining impediment to that is a call to
setLastTid(), which is defined in tid.c but declared in heapam.h.
That doesn't seem like a particularly accurate location, it doesn't
really have that much to do with heap. It seems more like a general
executor facility or something. Does anybody have a good idea where to
put the declaration?
Looking at how this function is used, lead to some confusion on my part.
We currently call setLastTid in ExecInsert():
if (canSetTag)
{
(estate->es_processed)++;
setLastTid(&slot->tts_tid);
}
And Current_last_tid, the variable setLastTid sets, is only used in
currtid_byreloid():
Datum
currtid_byreloid(PG_FUNCTION_ARGS)
{
Oid reloid = PG_GETARG_OID(0);
ItemPointer tid = PG_GETARG_ITEMPOINTER(1);
ItemPointer result;
Relation rel;
AclResult aclresult;
Snapshot snapshot;
result = (ItemPointer) palloc(sizeof(ItemPointerData));
if (!reloid)
{
*result = Current_last_tid;
PG_RETURN_ITEMPOINTER(result);
}
I've got to say I'm a bit baffled by this interface. If somebody passes
in a 0 reloid, we just ignore the passed in tid, and return the last tid
inserted into any table?
I then was even more baffled to find that there's no documentation of
this function, nor this special case behaviour, to be found
anywhere. Not in the docs (which don't mention the function, nor it's
special case behaviour for relation 0), nor in the code.
It's unfortunately used in psqlobdc:
else if ((flag & USE_INSERTED_TID) != 0)
printfPQExpBuffer(&selstr, "%s where ctid = (select
currtid(0, '(0,0)'))", load_stmt);
I gotta say, all that currtid code looks to me like it just should be
ripped out. It really doesn't make a ton of sense to just walk the tid
chain for a random tid - without an open snapshot, there's absolutely no
guarantee that you get back anything meaningful. Nor am I convinced
it's perfectly alright to just return the latest inserted tid for a
relation the user might not have any permission for.
OTOH, we probably can't just break psqlodbc, so we probably have to hold
our noses a bit longer and just move the prototype elsewhere? But I'm
inclined to just say that this functionality is going to get ripped out
soon, unless somebody from the odbc community works on making it make a
bit more sense (tests, docs at the very very least).
Greetings,
Andres Freund
