Re: [PATCHES] lastval()
Neil Conway wrote: Attached is a revised patch. Applied to HEAD. Thanks for the patch, Dennis. -Neil ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
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
Index: doc/src/sgml/func.sgml
===
RCS file: /var/lib/cvs/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.251
diff -c -r1.251 func.sgml
*** doc/src/sgml/func.sgml 6 Jun 2005 16:29:01 - 1.251
--- doc/src/sgml/func.sgml 7 Jun 2005 04:05:29 -
***
*** 6488,6493
--- 6488,6496
currval
+lastval
+
+
setval
***
*** 6519,6524
--- 6522,6533
currval(text)
bigint
+ Return value most recently obtained with
+ nextval for specified sequence
+
+
+ lastval()
+ bigint
Return value most recently obtained with nextval
***
*** 6588,6593
--- 6597,6618
+ lastval
+
+
+ Return the value most recently returned by
+ nextval in the current session. This function is
+ identical to currval, except that instead
+ of taking the sequence name as an argument it fetches the
+ value of the last sequence that nextval
+ was used on in the current session. It is an error to call
+ lastval if nextval
+ has not yet been called in the current session.
+
+
+
+
+
setval
Index: src/backend/commands/sequence.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/commands/sequence.c,v
retrieving revision 1.122
diff -c -r1.122 sequence.c
*** src/backend/commands/sequence.c 6 Jun 2005 20:22:57 - 1.122
--- src/backend/commands/sequence.c 7 Jun 2005 03:52:05 -
***
*** 24,29
--- 24,30
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/resowner.h"
+ #include "utils/syscache.h"
/*
***
*** 68,74
--- 69,81
static SeqTable seqtab = NULL; /* Head of list of SeqTable items */
+ /*
+ * last_used_seq is updated by nextval() to point to the last used
+ * sequence.
+ */
+ static SeqTableData *last_used_seq = NULL;
+ static void acquire_share_lock(Relation seqrel, SeqTable seq);
static void init_sequence(RangeVar *relation,
SeqTable *p_elm, Relation *p_rel);
static Form_pg_sequence read_info(SeqTable elm, Relation rel, Buffer *buf);
***
*** 400,405
--- 407,413
if (elm->last != elm->cached) /* some numbers were cached */
{
+ last_used_seq = elm;
elm->last += elm->increment;
relation_close(seqrel, NoLock);
PG_RETURN_INT64(elm->last);
***
*** 521,526
--- 529,536
elm->last = result; /* last returned number */
elm->cached = last; /* last fetched number */
+ last_used_seq = elm;
+
START_CRIT_SECTION();
/* XLOG stuff */
***
*** 602,607
--- 612,653
PG_RETURN_INT64(result);
}
+ Datum
+ lastval(PG_FUNCTION_ARGS)
+ {
+ Relation seqrel;
+ int64 result;
+
+ if (last_used_seq == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("lastval is not yet defined in this session")));
+
+ /* Someone may have dropped the sequence since the last nextval() */
+ if (!SearchSysCacheExists(RELOID,
+ ObjectIdGetDatum(last_used_seq->relid),
+ 0, 0, 0))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("lastval is not yet defined in this session")));
+
+ seqrel = relation_open(last_used_seq->relid, NoLock);
+ acquire_share_lock(seqrel, last_used_seq);
+
+ /* nextval() must have already been called for this sequence */
+ Assert(last_used_seq->increment != 0);
+
+ if (pg_class_aclcheck(last_used_seq->relid, GetUserId(), ACL_SELECT) != ACLCHECK_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for sequence %s",
+ RelationGetRelationName(seqrel;
+
+ result = last_used_seq->last;
+ relation_close(seqrel, NoLock);
+ PG_RETURN_INT64(res
Re: [PATCHES] lastval()
lastval() returns the last value produced by nextval() or setval() in the current session. I'm in favour of that definition... Chris ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] lastval()
Yes please... ... That's exactly what I've been asking for > -Original Message- > From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] On Behalf Of Neil Conway > Sent: Monday, June 06, 2005 12:18 PM > To: Dennis Bjorklund > Cc: pgsql-patches@postgresql.org > Subject: Re: [PATCHES] lastval() > > 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 > > ---(end of > broadcast)--- > TIP 6: Have you searched our list archives? > >http://archives.postgresql.org > > ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] lastval()
Neil Conway <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
Neil Conway <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
Neil Conway <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
At 2005-06-06 12:18:22 +1000, [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] lastval()
> > I want the feature. Is useful for PHP ... > Ditto! ... John ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] lastval()
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
---(end of broadcast)---
TIP 8: explain analyze is your friend
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] lastval()
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
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] lastval()
"John Hansen" <[EMAIL PROTECTED]> writes: >> Hmm, if your app can build any of them at an arbitrary point, >> you have a rather serious problem, I'd say. The apps I've >> seen build either kind at each call site of such runquery(). > Actually, the app that I am referring to, does just that. > However, in some instances, the only difference between two queries, would be > the table name. > A primary, with a serial as primary key, and a secondary without a serial. > Inserting into the secondary, gives no sequence value, and thus currval() (or > lastval()) would fail. > I solved the error issue by hardcoding the table names available in > runquery(), such that currval() is only called when the table is one of those > containing a serial primary key. Not very elegant, which is why I'd like to > see at least the lastval(false) method implemented. I continue to find this argument completely unconvincing. If we provide a function that works like that, then the result will be that whenever you insert into the secondary table, your application will obtain and use a completely WRONG answer --- namely the sequence value from some other table entirely. If your code is able to not do anything wrong as a result, it seems to me it could refrain from calling the function in the first place. regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] lastval()
Alvaro Herrera wrote: > Sent: Thursday, May 12, 2005 6:36 AM > To: John Hansen > Cc: Bruce Momjian; Neil Conway; Dennis Bjorklund; > pgsql-patches@postgresql.org > Subject: Re: [PATCHES] lastval() > > On Thu, May 12, 2005 at 04:58:54AM +1000, John Hansen wrote: > > Alvaro Herrera wrote: > > > > Actually, having it throw an error would be helpful, because then > > > you can find in the application which calls should be replaced by > > > the generic runquery() that has to return nothing versus the one > > > that has to return a sequence value. So porting is a little more > > > involved but more useful in the end. > > > > Indeed, but my point was that often it is going in the too > hard basket. > > Not that I disagree, but how do you predetermine which > queries would > > throw an error if they're built dynamically? > > Hmm, if your app can build any of them at an arbitrary point, > you have a rather serious problem, I'd say. The apps I've > seen build either kind at each call site of such runquery(). Actually, the app that I am referring to, does just that. However, in some instances, the only difference between two queries, would be the table name. A primary, with a serial as primary key, and a secondary without a serial. Inserting into the secondary, gives no sequence value, and thus currval() (or lastval()) would fail. I solved the error issue by hardcoding the table names available in runquery(), such that currval() is only called when the table is one of those containing a serial primary key. Not very elegant, which is why I'd like to see at least the lastval(false) method implemented. > > -- > Alvaro Herrera () > "La fuerza no está en los medios físicos sino que reside en > una voluntad indomable" (Gandhi) ... John ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] lastval()
On Thu, May 12, 2005 at 04:58:54AM +1000, John Hansen wrote: > Alvaro Herrera wrote: > > Actually, having it throw an error would be helpful, because > > then you can find in the application which calls should be > > replaced by the generic runquery() that has to return nothing > > versus the one that has to return a sequence value. So > > porting is a little more involved but more useful in the end. > > Indeed, but my point was that often it is going in the too hard basket. > Not that I disagree, but how do you predetermine which queries would > throw an error if they're built dynamically? Hmm, if your app can build any of them at an arbitrary point, you have a rather serious problem, I'd say. The apps I've seen build either kind at each call site of such runquery(). -- Alvaro Herrera () "La fuerza no está en los medios físicos sino que reside en una voluntad indomable" (Gandhi) ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] lastval()
Alvaro Herrera wrote:
> Sent: Wednesday, May 11, 2005 10:46 PM
> To: John Hansen
> Cc: Bruce Momjian; Neil Conway; Dennis Bjorklund;
> pgsql-patches@postgresql.org
> Subject: Re: [PATCHES] lastval()
>
> On Wed, May 11, 2005 at 02:08:16PM +1000, John Hansen wrote:
>
> > Take for instance this (overly simplified) function used in
> a program
> > that builds the query strings dynamically:
> >
> > int64 runquery(char *query) {
> > PQexec(query);
> > result = Pqexec("SELECT lastval()");
> > return result;
> > }
> >
> > The program expects this function to return the 'id' that was
> > inserted, or 0 if the table didn't contain a sequence or it
> wasn't an insert.
> >
> > Rewriting that would take a considerable effort.
>
> Actually, having it throw an error would be helpful, because
> then you can find in the application which calls should be
> replaced by the generic runquery() that has to return nothing
> versus the one that has to return a sequence value. So
> porting is a little more involved but more useful in the end.
Indeed, but my point was that often it is going in the too hard basket.
Not that I disagree, but how do you predetermine which queries would
throw an error if they're built dynamically?
>
> --
> Alvaro Herrera ()
>
>
---(end of broadcast)---
TIP 9: the planner will ignore your desire to choose an index scan if your
joining column's datatypes do not match
Re: [PATCHES] lastval()
On Wed, May 11, 2005 at 02:08:16PM +1000, John Hansen wrote:
> Take for instance this (overly simplified) function used in a program
> that builds the query strings dynamically:
>
> int64 runquery(char *query) {
> PQexec(query);
> result = Pqexec("SELECT lastval()");
> return result;
> }
>
> The program expects this function to return the 'id' that was inserted,
> or 0 if the table didn't contain a sequence or it wasn't an insert.
>
> Rewriting that would take a considerable effort.
Actually, having it throw an error would be helpful, because then you
can find in the application which calls should be replaced by the
generic runquery() that has to return nothing versus the one that has to
return a sequence value. So porting is a little more involved but
more useful in the end.
--
Alvaro Herrera ()
---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
Tom Lane wrote: > Sent: Wednesday, May 11, 2005 3:41 PM > To: John Hansen > Cc: Neil Conway; Dennis Bjorklund; pgsql-patches@postgresql.org > Subject: Re: [PATCHES] lastval() > > "John Hansen" <[EMAIL PROTECTED]> writes: > > I'm all for it. Even more so if the 'currval(void) called before > > nextval(seq_name)' error message could be supressed by a > GUC variable > > and return 0 instead. > > I really have a hard time seeing the argument why that > condition does not mean "your application is broken and you > should fix it". I'm _not_ saying that, I'm saying that the ports that would be a breeze with this behaviour, probably won't happen without. Which is a shame. Also note that I'm not suggesting this be the default behaviour. I'm not even suggesting it be a configurable permanent (in fact it probably shouldn't be configurable thru postgresql.conf), but merely a SET parameter, that you can set prior to executing lastval(), or perhaps as lastval(false/true); > Much less why "0" is the correct response --- it's barely > conceivable that you could persuade me that NULL is ok, but > never a value that is a valid sequence value. NULL would do fine. > regards, tom lane > > ... John ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] lastval()
"John Hansen" <[EMAIL PROTECTED]> writes: > I'm all for it. Even more so if the 'currval(void) called before > nextval(seq_name)' error message could be supressed by a GUC variable > and return 0 instead. I really have a hard time seeing the argument why that condition does not mean "your application is broken and you should fix it". Much less why "0" is the correct response --- it's barely conceivable that you could persuade me that NULL is ok, but never a value that is a valid sequence value. regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] lastval()
Bruce Momjian 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] lastval()
Neil Conway wrote: > Sent: Wednesday, May 11, 2005 2:13 PM > To: John Hansen > Cc: Dennis Bjorklund; pgsql-patches@postgresql.org > Subject: Re: [PATCHES] lastval() > > John Hansen wrote: > > Even more so if the 'currval(void) called before nextval(seq_name)' > > error message could be supressed by a GUC variable and return 0 > > instead. > > I don't think that's a good idea at all. It seems > unnecessary, it complicates the definition of the operation, > and worse, "0" is a valid return value for currval(), so it > would also be ambiguous. K, No mysql-ism for postgresql then :) (pardon my french) > -Neil > > ... John ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] lastval()
John Hansen wrote: Even more so if the 'currval(void) called before nextval(seq_name)' error message could be supressed by a GUC variable and return 0 instead. I don't think that's a good idea at all. It seems unnecessary, it complicates the definition of the operation, and worse, "0" is a valid return value for currval(), so it would also be ambiguous. -Neil ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] lastval()
Bruce Momjian wrote:
> Sent: Wednesday, May 11, 2005 1:45 PM
> To: John Hansen
> Cc: Neil Conway; Dennis Bjorklund; pgsql-patches@postgresql.org
> Subject: Re: [PATCHES] lastval()
>
> John Hansen wrote:
> > > > > What do people think of this idea? (Tom seems
> opposed, I'm just
> > > > > wondering if there are other opinions out there.)
> > > >
> > > > I'm all for it. Even more so if the 'currval(void)
> called before
> > > > nextval(seq_name)' error message could be supressed by a
> > > GUC variable
> > > > and return 0 instead.
> > >
> > > Why zero and no error?
> >
> > That's the exact behaviour of the "other database's" equivalent.
> > Makes porting easier, and avoids hugely annoying error
> messages in the
> > logfiles.
>
> I think we would have to throw an error. It is hard to see
> how zero is a valid return value. If you are getting too
> many errors in your logs, fix the code.
Yes, that would be ideal, but most proting efforts seem to stall at the
'less trivial' problems.
Take for instance this (overly simplified) function used in a program
that builds the query strings dynamically:
int64 runquery(char *query) {
PQexec(query);
result = Pqexec("SELECT lastval()");
return result;
}
The program expects this function to return the 'id' that was inserted,
or 0 if the table didn't contain a sequence or it wasn't an insert.
Rewriting that would take a considerable effort.
Now, I'm not saying this design is not broken to begin with, or that it
shouldn't be rewritten anyways,... I'm saying that for many applications
it won't happen because it's easier to just use another database
instead.
Besides, what's wrong with _knowingly_ telling the backend that: hey, I
know this query might throw an error, so just throw me a 0 instead if
you can't compute it?
... John
---(end of broadcast)---
TIP 9: the planner will ignore your desire to choose an index scan if your
joining column's datatypes do not match
Re: [PATCHES] lastval()
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
---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] lastval()
John Hansen wrote: > > > > What do people think of this idea? (Tom seems opposed, I'm just > > > > wondering if there are other opinions out there.) > > > > > > I'm all for it. Even more so if the 'currval(void) called before > > > nextval(seq_name)' error message could be supressed by a > > GUC variable > > > and return 0 instead. > > > > Why zero and no error? > > That's the exact behaviour of the "other database's" equivalent. > Makes porting easier, and avoids hugely annoying error messages in the > logfiles. I think we would have to throw an error. It is hard to see how zero is a valid return value. If you are getting too many errors in your logs, fix the code. -- 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] lastval()
Bruce Momjian wrote: > Sent: Wednesday, May 11, 2005 1:27 PM > To: John Hansen > Cc: Neil Conway; Dennis Bjorklund; pgsql-patches@postgresql.org > Subject: Re: [PATCHES] lastval() > > John Hansen wrote: > > Neil Conway wrote: > > > Sent: Wednesday, May 11, 2005 10:56 AM > > > To: Dennis Bjorklund > > > Cc: pgsql-patches@postgresql.org > > > Subject: Re: [PATCHES] lastval() > > > > > > 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'm all for it. Even more so if the 'currval(void) called before > > nextval(seq_name)' error message could be supressed by a > GUC variable > > and return 0 instead. > > Why zero and no error? That's the exact behaviour of the "other database's" equivalent. Makes porting easier, and avoids hugely annoying error messages in the logfiles. > > -- > 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 > > ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] lastval()
Abhijit Menon-Sen wrote: > At 2005-05-11 10:55:37 +1000, [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] lastval()
John Hansen wrote: > Neil Conway wrote: > > Sent: Wednesday, May 11, 2005 10:56 AM > > To: Dennis Bjorklund > > Cc: pgsql-patches@postgresql.org > > Subject: Re: [PATCHES] lastval() > > > > 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'm all for it. Even more so if the 'currval(void) called before > nextval(seq_name)' error message could be supressed by a GUC variable > and return 0 instead. Why zero and no error? -- 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] lastval()
Abhijit Menon-Sen said: > At 2005-05-11 10:55:37 +1000, [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] lastval()
Neil Conway wrote: > Sent: Wednesday, May 11, 2005 10:56 AM > To: Dennis Bjorklund > Cc: pgsql-patches@postgresql.org > Subject: Re: [PATCHES] lastval() > > 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'm all for it. Even more so if the 'currval(void) called before nextval(seq_name)' error message could be supressed by a GUC variable and return 0 instead. > 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 > > ---(end of > broadcast)--- > TIP 7: don't forget to increase your free space map settings > > .. John ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] lastval()
At 2005-05-11 10:55:37 +1000, [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] lastval()
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
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] lastval()
Neil Conway <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] lastval()
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 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] lastval()
Tom Lane wrote: > Sent: Monday, May 09, 2005 8:37 AM > To: Dennis Bjorklund > Cc: pgsql-patches@postgresql.org > Subject: Re: [PATCHES] lastval() > > Dennis Bjorklund <[EMAIL PROTECTED]> 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. > And you threw out the idea when I proposed it more than a year ago. However I'd still like to see it, tho in the same shape as 'the other database'. Eg, succeeding when nextval has not been called, returning 0. Thus, my suggestion for last_insert_id() still stands. > regards, tom lane > > ---(end of > broadcast)--- > TIP 1: subscribe and unsubscribe commands go to > [EMAIL PROTECTED] > > ... John ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] lastval()
Dennis Bjorklund <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
