setLastTid() and currtid()

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

setLastTid() and currtid()

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

Re: setLastTid() and currtid()

Inoue, Hiroshi-2
Hi Andres,
Sorry for the 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().

regards,
Hiroshi Inoue

> 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

---
このメールは、AVG によってウイルス チェックされています。
http://www.avg.com



Reply | Threaded
Open this post in threaded view
|

Re: setLastTid() and currtid()

Andres Freund
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


Reply | Threaded
Open this post in threaded view
|

Re: setLastTid() and currtid()

Alvaro Herrera-9
In reply to this post by Andres Freund
On 2019-Apr-11, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2019-03-27 10:01:08 +0900, Inoue, Hiroshi wrote:
> >> 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?
>
> I think removing it after feature freeze is not something to do,
> but +1 for nuking it as soon as the v13 branch opens.  Unless
> there's some important reason we need it to be gone in v12?

Umm ... I'm not sure I agree.  We're in feature freeze, not code freeze,
and while we're not expecting to have any new feature patches pushed,
cleanup for features that did make the cut is still fair game.  As I
understand, this setLastTid stuff would cause trouble if used with a
non-core table AM.  Furthermore, if nothing uses it, what's the point of
keeping it?

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: setLastTid() and currtid()

Andres Freund
In reply to this post by Andres Freund
Hi,

On 2019-04-11 13:27:03 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2019-03-27 10:01:08 +0900, Inoue, Hiroshi wrote:
> >> 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?
>
> I think removing it after feature freeze is not something to do,
> but +1 for nuking it as soon as the v13 branch opens.  Unless
> there's some important reason we need it to be gone in v12?

No, I don't think there really is. They're bogus and possibly a bit
dangerous, but that's not really new.

I was mostly just reminded of this when Heikki asked me to improve the
documentation for heap_get_latest_tid/table_get_latest_tid() - and I was
briefly wondering whether we could just nuke the whole functionality.
But it's still used in nodeTidscan.c:

                /*
                 * For WHERE CURRENT OF, the tuple retrieved from the cursor might
                 * since have been updated; if so, we should fetch the version that is
                 * current according to our snapshot.
                 */
                if (node->tss_isCurrentOf)
                        table_get_latest_tid(heapRelation, snapshot, &tid);

If we were able to just get rid of that I think there'd have been a
strong case for removing $subject in v12, to avoid exposing something to
new AMs that we're going to nuke in v13.

The only other reason I can see is that there's literally no use for
them (bogus and only used by pgodbc when targeting <= 8.2), and that
they cost a bit of performance and are the only reason heapam.h is still
included in nodeModifyTable.h (hurting my pride).   But that's probably
not sufficient reason.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: setLastTid() and currtid()

Andres Freund
In reply to this post by Alvaro Herrera-9
Hi,

On 2019-04-11 13:52:08 -0400, Alvaro Herrera wrote:
> As I understand, this setLastTid stuff would cause trouble if used
> with a non-core table AM.

I'm not sure there'd actually be trouble. I mean, what it does for heap
is basically meaningless already, so it's not going to be meaningfully
worse for any other table AM.  It's an undocumented odd interface, whose
implementation is also ugly, and that'd be a fair reason on its own to
rip it out though.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: setLastTid() and currtid()

Michael Paquier-2
In reply to this post by Andres Freund
On Thu, Apr 11, 2019 at 02:06:13PM -0400, Tom Lane wrote:
> Yeah, if we could simplify the tableam API, that would be sufficient
> reason to remove the stuff in v12, IMO.  But if it is outside of that
> API, I'd counsel waiting till v13.

Yes, I agree that simplifying the table AM interface would be a reason
fine enough to delete this code for v12.  If not, v13 sounds better at
this stage.
--
Michael

signature.asc (849 bytes) Download Attachment