Обсуждение: BUG #15198: nextval() accepts tables/indexes when adding a default toa column

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

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

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

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], 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