BUG #15198: nextval() accepts tables/indexes when adding a default to a column

On 5/16/18 05:29, PG Bug reporting form wrote:

bugtest=# CREATE TABLE demo(i int default nextval('demo') PRIMARY KEY);
CREATE TABLE
bugtest=# ALTER TABLE demo ADD COLUMN j int default nextval('demo_pkey');
ALTER TABLE
bugtest=# \d demo
Table "public.demo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+--------------------------------
i | integer | | not null | nextval('demo'::regclass)
j | integer | | | nextval('demo_pkey'::regclass)
Indexes:
"demo_pkey" PRIMARY KEY, btree (i)

bugtest=# INSERT INTO demo (i, j) VALUES (1,1);
INSERT 0 1
bugtest=# INSERT INTO demo (i, j) VALUES (DEFAULT, DEFAULT);
ERROR: 42809: "demo" is not a sequence
LOCATION: init_sequence, sequence.c:1139

You are right that this is not optimal behavior. I'm not sure if it's
worth fixing, however. (Introduce a regsequence type to use in place of
regclass?)

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, May 16, 2018 at 7:00 AM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

bugtest=# INSERT INTO demo (i, j) VALUES (1,1);
INSERT 0 1
bugtest=# INSERT INTO demo (i, j) VALUES (DEFAULT, DEFAULT);
ERROR: 42809: "demo" is not a sequence
LOCATION: init_sequence, sequence.c:1139

You are right that this is not optimal behavior. I'm not sure if it's
worth fixing, however. (Introduce a regsequence type to use in place of
regclass?)

​There is a big note on the functions-sequence page in the docs covering
late binding and text. A addition like below is an acceptable solution for
me:

Additionally, since pg_class contains objects other than sequences it is
possible to specify a default that, at runtime, points to a non-sequence
object and provokes an error. (i.e., the type of the pointed to pg_class
record is not checked during the cast but during function evaluation).

David J.

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 5/16/18 05:29, PG Bug reporting form wrote:

ERROR: 42809: "demo" is not a sequence

You are right that this is not optimal behavior. I'm not sure if it's
worth fixing, however. (Introduce a regsequence type to use in place of
regclass?)

That's about what we'd have to do, and it seems like far more
infrastructure than the problem is worth. All you're accomplishing
is to emit the same error at a different time, and for that you need
a named, documented data type.

Furthermore, there are plenty of other places with a similar claim
to trouble, but I can't see inventing different variants of regclass
to enforce all the different restrictions you could wish for:

* pg_index_has_property could wish for a regindex type, perhaps
(and brin_summarize_new_values could wish for a restriction to
BRIN indexes, or gin_clean_pending_list to GIN indexes)

* pg_relation_filenode could wish for a restriction to relation
kinds that have storage

* pg_relation_is_publishable doubtless has some other relkind
restriction

* I didn't even check functions that currently take OID rather
than regclass

regards, tom lane

On 5/16/18 10:14, Tom Lane wrote:

That's about what we'd have to do, and it seems like far more
infrastructure than the problem is worth. All you're accomplishing
is to emit the same error at a different time, and for that you need
a named, documented data type.

In this case, they are putting the erroneous call into a column default,
so the difference ends up being getting the error at setup time versus
at run time, which is a difference of significance. However, that kind
of manual fiddling should be rare, and it's certainly not the only way
to create run time errors from complex default expressions.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 16 May 2018 at 16:20, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

In this case, they are putting the erroneous call into a column default,
so the difference ends up being getting the error at setup time versus
at run time, which is a difference of significance.

Yes, I'm not particularly concerned with nextval taking a regclass as
an argument, and
therefore raising this error, but I'd rather have this error at DDL
time than at DML time.

I don't know how hard it would be to implement, but say, calling
currval(regclass) when
a default is defined should already throw this error at DDL time.

Or, when registering the default in the catalog, we verify that it is
actually a sequence:

Note: I'm not a C coder, so read it as pseudo-code please.

--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2059,6 +2059,9 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
        defobject.objectId = attrdefOid;
        defobject.objectSubId = 0;

+       if (!IsSequence( find_oid_referenced (defobject) ) )
+               elog(ERROR, "Column defaults can only depend on sequences")
+
        heap_close(adrel, RowExclusiveLock);

/* now can free some of the stuff allocated above */

but again, I've only seen this once, so the value of adding this check
seems very limited

On Wed, May 16, 2018 at 11:41 PM, Feike Steenbergen <
feikesteenbergen@gmail.com> wrote:

+       if (!IsSequence( find_oid_referenced (defobject) ) )
+               elog(ERROR, "Column defaults can only depend on sequences")

​Except column defaults can depends on lots of things - its only if the
column default happens to invoke nextval that the specific type of object
being passed to nextval needs to be a sequence.

You might be able to stick "something" in
the recordDependencyOnExpr(&defobject, expr, NIL, DEPENDENCY_NORMAL); call
(have gone and found that code...) but catalog/heap.c:: StoreAttrDefault itself
doesn't operate at the level of detail.

Ultimately you'd have to add a hack for the function name nextval...

David J.
​

Hi,

On 2018-05-17 08:41:53 +0200, Feike Steenbergen wrote:

On 16 May 2018 at 16:20, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

In this case, they are putting the erroneous call into a column default,
so the difference ends up being getting the error at setup time versus
at run time, which is a difference of significance.

Yes, I'm not particularly concerned with nextval taking a regclass as
an argument, and
therefore raising this error, but I'd rather have this error at DDL
time than at DML time.

I don't know how hard it would be to implement, but say, calling
currval(regclass) when
a default is defined should already throw this error at DDL time.

Or, when registering the default in the catalog, we verify that it is
actually a sequence:

These alternatives seem like they're not an improvement. I don't think
it's worth doing anything here.

Greetings,

Andres Freund

On 2018-May-17, Andres Freund wrote:

These alternatives seem like they're not an improvement. I don't think
it's worth doing anything here.

I agree.

If our nextval was less opaque, it'd be worth doing better. I mean
something like

CREATE TABLE tt (
col integer DEFAULT someseq.nextval
...
)

which I think has been proposed over the years (and ultimately rejected;
and even if implemented[1]That syntax currently gets this funny error:, this would not prevent our current syntax
from being accepted). But we've stuck with the function-call syntax for
better or worse. Let's live with it.

[1]: That syntax currently gets this funny error:

alvherre=# create table ff (a int default seq.nextval);
ERROR: missing FROM-clause entry for table "seq"

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, May 17, 2018 at 12:36:31PM -0400, Alvaro Herrera wrote:

[1] That syntax currently gets this funny error:

alvherre=# create table ff (a int default seq.nextval);
ERROR: missing FROM-clause entry for table "seq"

Which may be a parser problem as well seeing how CONSTR_DEFAULT gets
created using an expression?
--
Michael