Re: 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