Horton

Thread

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

    PG Bug reporting form <noreply@postgresql.org> — 2018-05-16T09:29:25Z

    The following bug has been logged on the website:

Bug reference:      15198
Logged by:          Feike Steenbergen
Email address:      feikesteenbergen@gmail.com
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

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-05-16T14:00:02Z

    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

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

    David G. Johnston <david.g.johnston@gmail.com> — 2018-05-16T14:14:36Z

    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.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-16T14:14:39Z

    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

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-05-16T14:20:19Z

    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

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

    Feike Steenbergen <feikesteenbergen@gmail.com> — 2018-05-17T06:41:53Z

    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

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

    David G. Johnston <david.g.johnston@gmail.com> — 2018-05-17T12:25:54Z

    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.
​

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

    Andres Freund <andres@anarazel.de> — 2018-05-17T16:21:19Z

    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

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-05-17T16:36:31Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2018-05-18T01:12:11Z

    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