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]

Reply via email to