Re: lastval() - Mailing list pgsql-patches

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