SEO

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 majordomo@postgresql.org

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

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

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

---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org

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...5/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

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

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

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

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

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

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

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

---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings