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