BUG #15579: Adding a column with default from configuration parameter fails on 11.1

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

BUG #15579: Adding a column with default from configuration parameter fails on 11.1

PG Doc comments form
The following bug has been logged on the website:

Bug reference:      15579
Logged by:          Fredrik Widlert
Email address:      [hidden email]
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.

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15579: Adding a column with default from configuration parameter fails on 11.1

Dmitry Dolgov
> On Mon, Jan 7, 2019 at 1:33 PM PG Bug reporting form <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15579: Adding a column with default from configuration parameter fails on 11.1

Tom Lane-2
In reply to this post by PG Doc comments form
=?utf-8?q?PG_Bug_reporting_form?= <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15579: Adding a column with default from configuration parameter fails on 11.1

Andrew Dunstan

On 1/7/19 9:57 AM, Tom Lane wrote:

> =?utf-8?q?PG_Bug_reporting_form?= <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

Re: BUG #15579: Adding a column with default from configuration parameter fails on 11.1

Andrew Gierth
In reply to this post by PG Doc comments form
>>>>> "PG" == PG Bug reporting form <[hidden email]> 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)

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15579: Adding a column with default from configuration parameter fails on 11.1

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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15579: Adding a column with default from configuration parameter fails on 11.1

Andres Freund
On 2019-01-07 16:01:43 -0500, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15579: Adding a column with default from configuration parameter fails on 11.1

Andrew Dunstan

On 1/7/19 4:11 PM, Andres Freund wrote:

> On 2019-01-07 16:01:43 -0500, Tom Lane wrote:
>> Andrew Dunstan <[hidden email]> 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