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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

PG Bug reporting form
The following bug has been logged on the website:

Bug reference:      15198
Logged by:          Feike Steenbergen
Email address:      [hidden email]
PostgreSQL version: 10.4
Operating system:   CentOS Linux release 7.5.1804 (Core)
Description:        

We recently ran into a surprise when vetting our schema's:

One of our tables had column with a DEFAULT pointing to nextval('table').
perhaps an example will clarify things:


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


I would expect when setting a default when specifying nextval,
that only sequences are allowed to be specified, but - as shown above -
tables or indexes are also accepted during creation of the default.

I'm unsure whether fixing this is desirable, as a pg_dump/restore
would not work for those databases that have their defaults pointing
to things other than tables.

The following query helped us identify all of these issues we had,
which was luckily only 1:

select distinct
   refobjid::regclass::text,
   attname,
   pg_get_expr(adbin, adrelid)
from
   pg_depend
join
   pg_attrdef on (refobjid=adrelid AND refobjsubid=adnum)
join
   pg_attribute on (refobjid=attrelid AND adnum=attnum)
cross join lateral
   regexp_replace(pg_get_expr(adbin, adrelid), 'nextval\(''(.*)''::.*',
'\1')
   as next_relation(next_relname)
join
   pg_class pc on (next_relname = pc.oid::regclass::text)
where
   pc.relkind != 'S';

 refobjid | attname |          pg_get_expr
----------+---------+--------------------------------
 demo     | i       | nextval('demo'::regclass)
 demo     | j       | nextval('demo_pkey'::regclass)
(2 rows)

regards,

Feike

Reply | Threaded
Open this post in threaded view
|

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

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

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

David G Johnston
On Wed, May 16, 2018 at 7:00 AM, Peter Eisentraut <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
In reply to this post by Peter Eisentraut-6
Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

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

Feike Steenbergen
On 16 May 2018 at 16:20, Peter Eisentraut
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

David G Johnston
On Wed, May 16, 2018 at 11:41 PM, Feike Steenbergen <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
In reply to this post by Feike Steenbergen
Hi,

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

> On 16 May 2018 at 16:20, Peter Eisentraut
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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], 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

Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment