Обсуждение: BUG #15579: Adding a column with default from configuration parameterfails on 11.1

The following bug has been logged on the website:

Bug reference:      15579
Logged by:          Fredrik Widlert
Email address:      fredrik.widlert@digpro.se
PostgreSQL version: 11.1
Operating system:   Ubuntu 18.04.1 LTS
Description:

We use  configuration parameters as default values in some tables,
like this:

create table t (x int, str varchar(50) default
current_setting('public.some_setting'));

This works in version 11.1 as well as in earlier versions, since we
call set_config before inserting data into the table.

However, creating the table first and then adding the column does not
work on 11.1. It used to work at least from version 9.3 to 10.

create table t (x int);
alter table t add c varchar(50) default
current_setting('public.some_setting');

On 11.1 (Ubuntu 11.1-1.pgdg18.04+1 on x86_64-pc-linux-gnu), this fails
with
ERROR:  unrecognized configuration parameter "public.some_setting"
unless we have called set_config in the session trying to add the column.

We expect to only have to call set_config in the session trying
to insert data into the table.

Is this a bug or pehaps an intended change? We tried googling but
didn't find anything which seemed relevant.

> On Mon, Jan 7, 2019 at 1:33 PM PG Bug reporting form <noreply@postgresql.org> wrote:
>
> Is this a bug or pehaps an intended change? We tried googling but
> didn't find anything which seemed relevant.

Well, I guess it was introduced here [1] for fast ALTER TABLE ADD COLUMN
feature:

    The default expression is evaluated at the time of the ALTER TABLE and the
    result stored in a new column (attmissingval) in pg_attribute

Although I wonder if this case with current_setting was taken into account.
Probably it doesn't make sense to use fast alter table if a table is empty.


[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=16828d5c0273b4fe5f10f42588005f16b415b2d8

=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> ... creating the table first and then adding the column does not
> work on 11.1. It used to work at least from version 9.3 to 10.

> create table t (x int);
> alter table t add c varchar(50) default
> current_setting('public.some_setting');
> ERROR:  unrecognized configuration parameter "public.some_setting"

I think this is a brown-paper-bag bug in the fast-column-default feature.
current_setting() is stable, and should certainly not be treated as a
fast default, but behold the test looks like this:

        /* If the DEFAULT is volatile we cannot use a missing value */
        if (colDef->missingMode && contain_volatile_functions((Node *) expr))
            colDef->missingMode = false;

Of course, it should be insisting that the expression be immutable,
not just that it not be volatile.

-       /* If the DEFAULT is volatile we cannot use a missing value */
-       if (colDef->missingMode && contain_volatile_functions((Node *) expr))
+       /* missingMode can only be used for immutable defaults */
+       if (colDef->missingMode && contain_mutable_functions((Node *) expr))
            colDef->missingMode = false;

            regards, tom lane

On 1/7/19 9:57 AM, Tom Lane wrote:
> =?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
>> ... creating the table first and then adding the column does not
>> work on 11.1. It used to work at least from version 9.3 to 10.
>> create table t (x int);
>> alter table t add c varchar(50) default
>> current_setting('public.some_setting');
>> ERROR:  unrecognized configuration parameter "public.some_setting"
> I think this is a brown-paper-bag bug in the fast-column-default feature.
> current_setting() is stable, and should certainly not be treated as a
> fast default, but behold the test looks like this:
>
>         /* If the DEFAULT is volatile we cannot use a missing value */
>         if (colDef->missingMode && contain_volatile_functions((Node *) expr))
>             colDef->missingMode = false;
>
> Of course, it should be insisting that the expression be immutable,
> not just that it not be volatile.
>
> -       /* If the DEFAULT is volatile we cannot use a missing value */
> -       if (colDef->missingMode && contain_volatile_functions((Node *) expr))
> +       /* missingMode can only be used for immutable defaults */
> +       if (colDef->missingMode && contain_mutable_functions((Node *) expr))
>             colDef->missingMode = false;
>
>



Not sure who should be wearing a paper bag here, but I doubt it's me.
The feature is working here as designed and documented:


    andrew=# set foo.bar = baz;
    SET
    andrew=# create table foo( a text);
    CREATE TABLE
    andrew=# insert into foo values('a');
    INSERT 0 1
    andrew=# alter table foo add column b text default
    current_setting('foo.bar');
    ALTER TABLE
    andrew=# select * from foo;
     a |  b 
    ---+-----
     a | baz
    (1 row)

    andrew=# select current_setting('foo.baz');
    ERROR:  unrecognized configuration parameter "foo.baz"
    andrew=# alter table foo add column c text default
    current_setting('foo.baz', true);
    ALTER TABLE
    andrew=# select * from foo;
     a |  b  | c
    ---+-----+---
     a | baz |
    (1 row)


Stable expressions are quite ok for fast defaults. The expression is
evaluated once when the ALTER TABLE is done and the result (not the
expression) is stored in the catalog. The reason we check for volatile
expressions is precisely because we don't want all the existing rows to
get a single value in that case. This was discussed during the Postgres
11 development cycle.


Note: regardless of fast default, if you're going to use current_setting
in a default expression, you probably should use the missing_ok = true
variant. Otherwise you'll get an error any time you insert using the
default if the setting is missing.


cheers


andrew

>>>>> "PG" == PG Bug reporting form <noreply@postgresql.org> writes:

 PG> However, creating the table first and then adding the column does
 PG> not work on 11.1. It used to work at least from version 9.3 to 10.

 PG> create table t (x int);
 PG> alter table t add c varchar(50) default
 PG> current_setting('public.some_setting');

This used to work ONLY if the table is empty, since the alter table
would evaluate the expression once per row (and hence not evaluate it if
there are no rows).

On PG 11, the new fast-default stuff will evaluate the default once, if
it's not volatile, even if the table is empty. So this is an intended
change.

If you know that the table is empty when you do the alter table, you can
do this, which works on any pg version:

alter table t add c varchar(50),
  alter column c set default current_setting('public.some_setting');

(if the table is not empty, then existing rows will get a null value in
column "c")

-- 
Andrew (irc:RhodiumToad)

Andrew Dunstan <andrew@dunslane.net> writes:
> Stable expressions are quite ok for fast defaults. The expression is
> evaluated once when the ALTER TABLE is done and the result (not the
> expression) is stored in the catalog. The reason we check for volatile
> expressions is precisely because we don't want all the existing rows to
> get a single value in that case. This was discussed during the Postgres
> 11 development cycle.

Hmm.

The issue here is that if the table is empty, the old behavior evaluated
the expression zero times during ALTER TABLE.  Now we evaluate it once,
and if that throws an error, that's a user-visible behavior change.

Perhaps it's okay to decide that that's an acceptable behavioral change,
but it makes this feature less transparent than it was supposed to be.

            regards, tom lane

On 2019-01-07 16:01:43 -0500, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > Stable expressions are quite ok for fast defaults. The expression is
> > evaluated once when the ALTER TABLE is done and the result (not the
> > expression) is stored in the catalog. The reason we check for volatile
> > expressions is precisely because we don't want all the existing rows to
> > get a single value in that case. This was discussed during the Postgres
> > 11 development cycle.
> 
> Hmm.
> 
> The issue here is that if the table is empty, the old behavior evaluated
> the expression zero times during ALTER TABLE.  Now we evaluate it once,
> and if that throws an error, that's a user-visible behavior change.
> 
> Perhaps it's okay to decide that that's an acceptable behavioral change,
> but it makes this feature less transparent than it was supposed to be.

It doesn't seem too hard to scan far enough to see whether there's a
single non-dead row. So we could fix this, if we wanted.

But I'm disinclined to think that it's worth doing so - and there could
be drawbacks, e.gg tables that are all-dead.  Given the IMO quite minor
behaviour change, I'm thus disinclined to "fix" this..

Greetings,

Andres Freund

On 1/7/19 4:11 PM, Andres Freund wrote:
> On 2019-01-07 16:01:43 -0500, Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> Stable expressions are quite ok for fast defaults. The expression is
>>> evaluated once when the ALTER TABLE is done and the result (not the
>>> expression) is stored in the catalog. The reason we check for volatile
>>> expressions is precisely because we don't want all the existing rows to
>>> get a single value in that case. This was discussed during the Postgres
>>> 11 development cycle.
>> Hmm.
>>
>> The issue here is that if the table is empty, the old behavior evaluated
>> the expression zero times during ALTER TABLE.  Now we evaluate it once,
>> and if that throws an error, that's a user-visible behavior change.
>>
>> Perhaps it's okay to decide that that's an acceptable behavioral change,
>> but it makes this feature less transparent than it was supposed to be.



That's true. But only slightly less transparent, I'd suggest :-)



> It doesn't seem too hard to scan far enough to see whether there's a
> single non-dead row. So we could fix this, if we wanted.
>
> But I'm disinclined to think that it's worth doing so - and there could
> be drawbacks, e.gg tables that are all-dead.  Given the IMO quite minor
> behaviour change, I'm thus disinclined to "fix" this..
>

agreed. It might be worth a doc note?


cheers


andrew