Ronan Dunklau <ronan.dunklau@aiven.io> writes:
> The problem the author wants to solve is the fact they don't have a way of
> returning the ids when using COPY FROM. Pre-allocating them and assigning them
> to the individual records before sending them via COPY FROM would solve that
> for them.
True.
I took a quick look at this patch and am not pleased at all with the
implementation. That loop in nextval_internal is okay performance-wise
today, since there's a small upper bound on the number of times it will
iterate. But with this patch, a user can trivially lock up a backend for
up to 2^63 iterations; let's just say that's longer than you want to wait.
There's not even a CHECK_FOR_INTERRUPTS() in the loop :-(. Even without
mistakes or deliberate DoS attempts, looping means holding the sequence
lock for longer than we really want to.
I think to seriously consider a feature like this, nextval_internal
would have to be rewritten so that it can advance the counter the
correct number of steps without using a loop. That would be quite a
headache, once you've dealt with integer overflow, cyclic sequences,
and suchlike complications, but it's probably do-able.
I don't think I agree with Ronan's upthread comment that
>> However, I don't think that returning only the last value is a sensible thing
>> to do. The client will need to know the details of the sequence to do anything
>> useful about this, especially it's increment, minvalue, maxvalue and cycle
>> options.
Most applications are probably quite happy to assume that they know the
sequence's static parameters, and those that aren't can easily fetch them
using existing facilities. So I don't think that returning them in this
function's result is really necessary.
I've got no strong opinion about this bit:
> As suggested upthread, returning a resultset would probably be better.
There are use-cases for that, sure, but there are also use-cases for
returning just the first or just the last value --- I'd think "just the
first" is the more common need of those two. Aggregating over a resultset
is a remarkably inefficient answer when that's what you want.
In any case, "nextval()" is an increasingly poor name for these different
definitions, so I counsel picking some other name instead of overloading
nextval(). "nextvals()" would be a pretty good choice for the resultset
case, I think.
regards, tom lane