lastval()

Dennis Bjorklund <db@zigo.dhs.org> writes:

Here is a small patch that implements a function lastval() that
works just like currval() except that it give the current
value of the last sequence used by nextval().

Why is that a good idea? In a complex application it'd be awfully easy
to break logic that depends on such a thing.

regards, tom lane

Tom Lane wrote:

Why is that a good idea? In a complex application it'd be awfully easy
to break logic that depends on such a thing.

True, but I think it offers a usefully concise syntax for simpler
applications. Perhaps the documentation should be amended to mention the
potential risks? (e.g. additional nextval() calls in between the
nextval() you are interested in and the lastval()).

-Neil

Neil Conway <neilc@samurai.com> writes:

Tom Lane wrote:

Why is that a good idea? In a complex application it'd be awfully easy
to break logic that depends on such a thing.

True, but I think it offers a usefully concise syntax for simpler
applications. Perhaps the documentation should be amended to mention the
potential risks?

Like, say, the sequence being deleted before the lastval call?

If I thought it was a good idea at all, I'd bother to criticize the
patch itself --- it's got some problems.

regards, tom lane

On Sun, 8 May 2005, Tom Lane wrote:

Why is that a good idea? In a complex application it'd be awfully easy
to break logic that depends on such a thing.

Of course it can break. currval() can also break in a complex application
with triggers and rules that do things the developer does not expect.

There are however lots of cases where it is safe and useful. Not the least
when you want to port an application that uses similar features.

--
/Dennis Bj�rklund

On Sun, 8 May 2005, Tom Lane wrote:

Like, say, the sequence being deleted before the lastval call?

Then you get an error message. Same thing if you have revoked the rights
on the sequence before you call lastval().

In this case you can get a value that belong to a sequence that is
deleted. Is that better? To me it's a sign that something is wrong with
the application and an error is better to get. It's not like it's hard to
store a int64 value instead. It's in fact simpler, but I just don't see
that it solve any problem. If anything it can hide problems.

If you want lastval() to work just don't delete the sequence. It's as
simple as that.

The thing is that I don't care how it's implemented, it's the feature
itself that is more importent to decide if we want it or not. I'm sure the
code can be fixed so everybody is happy it in the end,

--
/Dennis Bj�rklund

On Mon, 9 May 2005, Dennis Bjorklund wrote:

The thing is that I don't care how it's implemented, it's the feature
itself that is more importent to decide if we want it or not. I'm sure the
code can be fixed so everybody is happy it in the end,

You could implement this on top of the current nextval without backend
changes.

Create a wrapper function on top of nextval that stores the value in a
temp table. Or a session variable if your PL language of choice has
them.

lastval would do a select on the temp table.

- Heikki

Heikki Linnakangas wrote:

On Mon, 9 May 2005, Dennis Bjorklund wrote:

The thing is that I don't care how it's implemented, it's the feature
itself that is more importent to decide if we want it or not. I'm
sure the
code can be fixed so everybody is happy it in the end,

You could implement this on top of the current nextval without backend
changes.

Create a wrapper function on top of nextval that stores the value in a
temp table. Or a session variable if your PL language of choice has them.

lastval would do a select on the temp table.

And this is making life easier for anybody? I don't think so.

cheers

andrew

Dennis Bjorklund wrote:

Here is a small patch that implements a function lastval() that
works just like currval() except that it give the current
value of the last sequence used by nextval().

What do people think of this idea? (Tom seems opposed, I'm just
wondering if there are other opinions out there.)

I like the concept, but I haven't looked at the code -- I'd be happy to
review the implementation, although I won't waste my time if most people
are opposed to the idea itself.

-Neil

Neil Conway wrote:

Dennis Bjorklund wrote:

Here is a small patch that implements a function lastval() that
works just like currval() except that it give the current
value of the last sequence used by nextval().

What do people think of this idea? (Tom seems opposed, I'm just
wondering if there are other opinions out there.)

I like the concept, but I haven't looked at the code -- I'd be happy to
review the implementation, although I won't waste my time if most people
are opposed to the idea itself.

I can't speak to the code but lastval is something that has been
requested by my customers many times.

Sincerely,

Joshua D. Drake

Neil Conway wrote:

Dennis Bjorklund wrote:

Here is a small patch that implements a function lastval() that
works just like currval() except that it give the current
value of the last sequence used by nextval().

What do people think of this idea? (Tom seems opposed, I'm just
wondering if there are other opinions out there.)

I like the idea of lastval, though I would rather see us just use
currval() with no argument for it, rather than invent a new function
name. It does the same as currval('last sequence called') right?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

I like the concept, but I haven't looked at the code -- I'd be happy to
review the implementation, although I won't waste my time if most people
are opposed to the idea itself.

It'd make implementing various PHP userland functions a real breeze...

Chris

Abhijit Menon-Sen said:

At 2005-05-11 10:55:37 +1000, neilc@samurai.com wrote:

Here is a small patch that implements a function lastval() [...]

What do people think of this idea? (Tom seems opposed, I'm just
wondering if there are other opinions out there.)

For what it's worth, I think it's a bad idea.

In the MySQL wire protocol (hi Dennis!), the "last insert id" is sent
along with every "OK" message, and the client can just keep the value
in memory. Users call a function to retrieve that value, rather than
issuing a "SELECT nextval()".

You can do both - they have an SQL level function as well as supporting it
at the protocol layer. See
http://dev.mysql.com/doc/mysql/en/information-functions.html

So the server-side lastval() function is not enough for any meaningful
compatibility. The client would also need to be changed to provide the
pgsql_last_insert_id() or a similar function (which could do a "SELECT
lastval()" internally).

In this situation -- where both client changes AND a server round-trip
are required -- what's the point of adding cruft to the server? Might
as well confine changes to the client, and use nextval to implement the
feature.

I don't believ it can be sensibly done by the client alone. Either it needs
something like this or it shouldn't be done at all.

By the way, what would lastval() do if an insert trigger inserts a row
into a table with another serial column?

or more than one? Yes, it's not good in certain circumstances. That doesn't
make it useless in all circumstances.

I'm not jumping out of my seat to have this. But as Joshua points out, it is
frequently requested.

cheers

andrew

At 2005-05-11 10:55:37 +1000, neilc@samurai.com wrote:

Here is a small patch that implements a function lastval() [...]

What do people think of this idea? (Tom seems opposed, I'm just
wondering if there are other opinions out there.)

For what it's worth, I think it's a bad idea.

In the MySQL wire protocol (hi Dennis!), the "last insert id" is sent
along with every "OK" message, and the client can just keep the value
in memory. Users call a function to retrieve that value, rather than
issuing a "SELECT nextval()".

So the server-side lastval() function is not enough for any meaningful
compatibility. The client would also need to be changed to provide the
pgsql_last_insert_id() or a similar function (which could do a "SELECT
lastval()" internally).

In this situation -- where both client changes AND a server round-trip
are required -- what's the point of adding cruft to the server? Might
as well confine changes to the client, and use nextval to implement
the feature.

By the way, what would lastval() do if an insert trigger inserts a row
into a table with another serial column?

-- ams

Abhijit Menon-Sen wrote:

At 2005-05-11 10:55:37 +1000, neilc@samurai.com wrote:

Here is a small patch that implements a function lastval() [...]

What do people think of this idea? (Tom seems opposed, I'm just
wondering if there are other opinions out there.)

For what it's worth, I think it's a bad idea.

In the MySQL wire protocol (hi Dennis!), the "last insert id" is sent
along with every "OK" message, and the client can just keep the value
in memory. Users call a function to retrieve that value, rather than
issuing a "SELECT nextval()".

So the server-side lastval() function is not enough for any meaningful
compatibility. The client would also need to be changed to provide the
pgsql_last_insert_id() or a similar function (which could do a "SELECT
lastval()" internally).

In this situation -- where both client changes AND a server round-trip
are required -- what's the point of adding cruft to the server? Might
as well confine changes to the client, and use nextval to implement
the feature.

By the way, what would lastval() do if an insert trigger inserts a row
into a table with another serial column?

It fails, just like it would fail now if the trigger inserted into the
same table that used the trigger, or a rule.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

At 2005-05-10 23:30:05 -0400, pgman@candle.pha.pa.us wrote:

By the way, what would lastval() do if an insert trigger inserts
a row into a table with another serial column?

It fails, just like it would fail now if the trigger inserted into
the same table that used the trigger, or a rule.

I don't understand what you mean. "Just like it would fail now"? It
doesn't exist yet, how can it fail? And how would it know when to
fail anyway, rather than return a wrong value?

-- ams

Abhijit Menon-Sen wrote:

At 2005-05-10 23:30:05 -0400, pgman@candle.pha.pa.us wrote:

By the way, what would lastval() do if an insert trigger inserts
a row into a table with another serial column?

It fails, just like it would fail now if the trigger inserted into
the same table that used the trigger, or a rule.

I don't understand what you mean. "Just like it would fail now"? It
doesn't exist yet, how can it fail? And how would it know when to
fail anyway, rather than return a wrong value?

Uh, if the table's sequence name is 'tab_x_seq', and you do
currval('tab_x_seq'), you will get the trigger or rule insert id in that
case.

So, currval() widens a problem we already have if the rule/trigger
inserts into the same table.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Abhijit Menon-Sen wrote:

By the way, what would lastval() do if an insert trigger inserts a row
into a table with another serial column?

It fails, just like it would fail now if the trigger inserted into the
same table that used the trigger, or a rule.

If it actually *failed* that would be one thing, but the proposed
patch does not do that. It looks more like the philosophy we usually
denigrate MySQL for, viz never fail even if you are staring a certain
application bug in the face.

regards, tom lane

Dennis Bjorklund wrote:

+ Datum
+ lastval(PG_FUNCTION_ARGS)
+ {
+ 	Relation	seqrel;
+ 	int64		result;
+ 
+ 	if (last_used_seq == NULL) {
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ 				 errmsg("nextval have not been used in the current session")));
+ 	}

"has not been" would be more better English.

+ 
+ 	seqrel = relation_open(last_used_seq->relid, NoLock);
+ 
+ 	acquire_share_lock (seqrel, last_used_seq);
+ 
+ 	if (pg_class_aclcheck(last_used_seq->relid, GetUserId(), ACL_SELECT) != ACLCHECK_OK)
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ 				 errmsg("permission denied for sequence with OID %d",
+ 						last_used_seq->relid)));

"%d" is always the wrong formatting sequence for OIDs (they are
unsigned, hence %u). But in any case user-visible error messages should
specify the name of the sequence, which you can get via
RelationGetRelationName(seqrel)

+ 
+ 	if (last_used_seq->increment == 0)	/* nextval/read_info were not called */
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ 				 errmsg("currval of sequence with OID %d is not yet defined in this session",
+ 						last_used_seq->relid)));

See above; however, when will this error actually be invoked? (The
comment is wrong, as last_used_seq won't be defined if nextval has not
been called.)

/*
+  * If we haven't touched the sequence already in this transaction,
+  * we need to acquire AccessShareLock.  We arrange for the lock to
+  * be owned by the top transaction, so that we don't need to do it
+  * more than once per xact.
+  */
+ static void
+ acquire_share_lock (Relation seqrel,
+ 					SeqTableData *data)

Confusing SeqTable and SeqTableData * is bad style. I personally don't
like putting pointers into typedefs, but since the PG code does this,
SeqTable should be used consistently rather than SeqTableData *. The
same applies to the definition of "last_used_seq".

Comments on behavior:

neilc=# select setval('foo', 500);
setval
--------
500
(1 row)

neilc=# select lastval();
lastval
---------
500
(1 row)

I'm not sure it's necessarily _wrong_ to update lastval() on both setval
and nextval, but if that's the behavior we're going to implement, it
should surely be documented.

neilc=# create sequence bar ; select nextval ('bar') ; drop sequence bar;
CREATE SEQUENCE
nextval
---------
1
(1 row)

DROP SEQUENCE
neilc=# select lastval();
ERROR: XX000: could not open relation with OID 16389

Needs a friendlier error message.

-Neil

On Sun, 2005-05-08 at 19:00 +0200, Dennis Bjorklund wrote:

Here is a small patch that implements a function lastval() that
works just like currval() except that it give the current
value of the last sequence used by nextval().

Have you had a chance to respin this patch per my earlier comments on
the implementation, Dennis?

-Neil

On Thu, 2 Jun 2005, Neil Conway wrote:

Here is a small patch that implements a function lastval() that

Have you had a chance to respin this patch per my earlier comments on
the implementation, Dennis?

I've been spending my free time on another project and I don't multitask
very well :-)

Anyway, let me take a look at it in a minute. My main comment is that it's
not the code that's the main thing to fix but to decide is if we want the
feature at all.

--
/Dennis Bj�rklund

On Thu, 19 May 2005, Neil Conway wrote:

+ 				 errmsg("currval of sequence with OID %d is not yet defined in this session",
+ 						last_used_seq->relid)));

See above; however, when will this error actually be invoked? (The
comment is wrong, as last_used_seq won't be defined if nextval has not
been called.)

Right, it shouldn't be called. It's only there because I kept all the
error cases from currval().

+ static void
+ acquire_share_lock (Relation seqrel,
+ 					SeqTableData *data)

Confusing SeqTable and SeqTableData * is bad style. I personally don't
like putting pointers into typedefs, but since the PG code does this,
SeqTable should be used consistently rather than SeqTableData *. The
same applies to the definition of "last_used_seq".

The reason why I use SeqTableData * is that this function and
last_used_seq is not a list like the SeqTable is but it's a pointer to a
single element in a SeqTable.

To me SeqTable semantically represents a linked list while SeqTableData is
one cell in the list and I wanted to make that visible in the types I
used. But whatever convention is used in the rest of pg should be
followed.

Comments on behavior:

neilc=# select setval('foo', 500);
setval
--------
500
(1 row)

neilc=# select lastval();
lastval
---------
500
(1 row)

I'm not sure it's necessarily _wrong_ to update lastval() on both setval
and nextval, but if that's the behavior we're going to implement, it
should surely be documented.

It's how currval works. You can do setval() on a sequence and then
currval() is defined.

neilc=# create sequence bar ; select nextval ('bar') ; drop sequence bar;
CREATE SEQUENCE
nextval
---------
1
(1 row)

DROP SEQUENCE
neilc=# select lastval();
ERROR: XX000: could not open relation with OID 16389

Needs a friendlier error message.

True.

--
/Dennis Bj�rklund

Anyway, let me take a look at it in a minute. My main comment is that it's
not the code that's the main thing to fix but to decide is if we want the
feature at all.

I want the feature. Is useful for PHP ...

Chris

If you're busy, I can clean this up and apply it.

I wonder if it would be better to have lastval() return the last value
returned by nextval() or setval() for the current session, regardless of
any intervening DROP SEQUENCE commands. This would simplify the
implementation (we can just store the int8 value produced by the last
nextval() / setval() rather than a pointer to the sequence object
itself), although it is debatable whether this behavior is more logical
or not. Comments?

-Neil

At 2005-06-06 12:18:22 +1000, neilc@samurai.com wrote:

Comments?

Could someone who likes this idea please write the documentation for it?
I'd really like to see a concise, complete description of the proposed
function, including potential caveats.

-- ams

Abhijit Menon-Sen wrote:

Could someone who likes this idea please write the documentation for it?

Dennis' original patch includes documentation updates and a description
of lastval():

http://archives.postgresql.org/pgsql-patches/2005-05/msg00059.php

I'd really like to see a concise, complete description of the proposed
function, including potential caveats.

lastval() returns the last value produced by nextval() or setval() in
the current session.

-Neil

Neil Conway <neilc@samurai.com> writes:

Abhijit Menon-Sen wrote:

I'd really like to see a concise, complete description of the proposed
function, including potential caveats.

lastval() returns the last value produced by nextval() or setval() in
the current session.

This definition is OK with me ... so long as it still includes the
phrase "an error occurs if no nextval or setval has occurred in the
current session". However it seemed that a number of people asking
for the feature wanted some-random-default to be returned instead.

Another question is why should setval affect the result? I don't
see the use-case for that offhand.

regards, tom lane

Tom Lane wrote:

This definition is OK with me ... so long as it still includes the
phrase "an error occurs if no nextval or setval has occurred in the
current session". However it seemed that a number of people asking
for the feature wanted some-random-default to be returned instead.

Right -- I think it definitely needs to return an error in that
situation. Per my earlier mail, the other debatable behavior is whether
lastval() should be defined if the sequence it would be returning the
currval() for has been subsequently dropped. I'm inclined to not return
an error here to simplify the implementation, but I'm open to objections.

Another question is why should setval affect the result? I don't
see the use-case for that offhand.

I'm not militant about it, but having setval() affect the result means
lastval() is more consistent with currval().

-Neil

Neil Conway <neilc@samurai.com> writes:

Per my earlier mail, the other debatable behavior is whether
lastval() should be defined if the sequence it would be returning the
currval() for has been subsequently dropped. I'm inclined to not return
an error here to simplify the implementation, but I'm open to objections.

I agree with that --- consider that you couldn't actually promise that
the sequence hadn't been dropped by the time the answer is returned,
anyway, unless you take out a lock on the sequence first. Which doesn't
seem like a behavior that is wanted here.

Another question is why should setval affect the result? I don't
see the use-case for that offhand.

I'm not militant about it, but having setval() affect the result means
lastval() is more consistent with currval().

That is a point; on the other side consider that the simpler definition
is better. Without a pretty strong reason to include setval in the list
of things that affect lastval, I'd leave it out. We just agreed above
that DROP SEQUENCE won't affect lastval, so you can hardly argue that
lastval will track currval's behavior exactly ...

regards, tom lane

Tom Lane wrote:

I agree with that --- consider that you couldn't actually promise that
the sequence hadn't been dropped by the time the answer is returned,
anyway, unless you take out a lock on the sequence first. Which doesn't
seem like a behavior that is wanted here.

The only objection I can see is that it arguably doesn't obey sequence
permissions: you need SELECT on a sequence to see its currval(), whereas
lastval() would return the same information without an equivalent
permission check.

-Neil

Neil Conway <neilc@samurai.com> writes:

The only objection I can see is that it arguably doesn't obey sequence
permissions: you need SELECT on a sequence to see its currval(), whereas
lastval() would return the same information without an equivalent
permission check.

Interesting point ... the nextval() could have been done inside a
SECURITY DEFINER function that has more privilege than the user of
lastval() has. I'm not sure that this is a very interesting information
leak, mind you, but it's something to consider.

You could fix that by remembering exactly which sequence produced
the lastval and checking its permissions ... of course that brings
back the issue of what happens if the sequence has been dropped ...

regards, tom lane

On Mon, 6 Jun 2005, Tom Lane wrote:

You could fix that by remembering exactly which sequence produced
the lastval and checking its permissions ...

That is what the implementation does. Instead of remembering the last
value it rememebers the last sequence (and it contains the last value for
that sequence).

The very reason for doing that in the first place was to mimic currval()
as much as possible wrt rights and existence of the sequence.

--
/Dennis Bj�rklund

lastval() returns the last value produced by nextval() or setval() in
the current session.

I'm in favour of that definition...

Chris

Neil Conway wrote:

If you're busy, I can clean this up and apply it.

Attached is a revised patch. Per subsequent discussion, I stuck with
your approach of keeping a pointer to the sequence object, rather than
just the last int64 produced by nextval(). That means we emit an error on:

CREATE SEQUENCE seq;
SELECT nextval('seq');
DROP SEQUENCE seq;
SELECT lastval();

It also means that setval() _does_ affect lastval(), and that we do
permission checks properly. Barring any objections I'll apply this later
tonight or tomorrow.

BTW, I noticed that the "permission denied" messages throughout the
source don't quote the name of the identifier for which permission has
been denied. This violates the error code conventions: "Use quotes
always to delimit file names, user-supplied identifiers, and other
variables that might contain words." Is there a reason for this?

-Neil

BTW, I noticed that the "permission denied" messages throughout the
source don't quote the name of the identifier for which permission has
been denied. This violates the error code conventions: "Use quotes
always to delimit file names, user-supplied identifiers, and other
variables that might contain words." Is there a reason for this?

Request: can we _please_ have the actual permission that is denied, and
the username it was denied to in the error messages?

It's really a pain when reviewing logs to see such an error, then not
know what it was for or who generated it...

Chris

Neil Conway wrote:

Attached is a revised patch.

Applied to HEAD. Thanks for the patch, Dennis.

-Neil