Re: setLastTid() and currtid()

Hi,

On 2019-03-27 10:01:08 +0900, Inoue, Hiroshi wrote:
> Hi Andres,
> Sorry for the late reply.

Not late at all. Sorry for *my* late reply :)

> On 2019/03/26 9:44, Andres Freund wrote:
> > 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);
>
> The above code remains only for PG servers whose version < 8.2.
> Please remove the code around setLastTid().

Does anybody else have concerns about removing this interface? Does
anybody think we should have a deprecation phase? Should we remove this
in 12 or 13?

Greetings,

Andres Freund