Обсуждение: setLastTid() and currtid()
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
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
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
Andres Freund <andres@anarazel.de> 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?
regards, tom lane
Andres Freund <andres@anarazel.de> 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?
regards, tom lane
On 2019-Apr-11, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
On 2019-Apr-11, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
Hi,
On 2019-04-11 13:27:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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
Hi,
On 2019-04-11 13:27:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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
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
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
Andres Freund <andres@anarazel.de> writes:
> On 2019-04-11 13:27:03 -0400, Tom Lane wrote:
>> 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:
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.
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> On 2019-04-11 13:27:03 -0400, Tom Lane wrote:
>> 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:
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.
regards, tom lane
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
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
Вложения
On Fri, Apr 12, 2019 at 1:44 PM Michael Paquier <michael@paquier.xyz> wrote: > > 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. Now we are in the dev of v13, so it's time to rip the functions out? Regards, -- Fujii Masao
On Fri, Apr 12, 2019 at 1:44 PM Michael Paquier <michael@paquier.xyz> wrote: > > 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. Now we are in the dev of v13, so it's time to rip the functions out? Regards, -- Fujii Masao
On Fri, Feb 7, 2020 at 05:24:12PM +0900, Fujii Masao wrote: > On Fri, Apr 12, 2019 at 1:44 PM Michael Paquier <michael@paquier.xyz> wrote: > > > > 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. > > Now we are in the dev of v13, so it's time to rip the functions out? Where are we on this? Can the functions be removed in PG 14? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Tue, Oct 13, 2020 at 02:12:53PM -0400, Bruce Momjian wrote: > Where are we on this? Can the functions be removed in PG 14? (Sent this message previously but it got lost after some cross-posting across two lists, issue fixed now.) I still have a patch lying around to do that, registered in the CF: https://commitfest.postgresql.org/30/2579/ And here is the latest status of the discussion, based on some study of the ODBC driver I have done: https://www.postgresql.org/message-id/20200626041155.GD1504@paquier.xyz I would rather gather any future discussions on the other thread. -- Michael