BUG #18097: Immutable expression not allowed in generated at

The following bug has been logged on the website:

Bug reference: 18097
Logged by: Jim Keener
Email address: jim(at)jimkeener(dot)com
PostgreSQL version: 15.0
Operating system: Linux
Description:

Given this table:

CREATE TABLE test_table (
id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
created_at timestamptz NOT NULL DEFAULT now()
);

The following work:

* alter table test_table add created_local_y text GENERATED ALWAYS AS
(EXTRACT(isoyear FROM created_at AT TIME ZONE 'America/New_York')) STORED;

* alter table test_table add created_local_w text GENERATED ALWAYS AS
(EXTRACT(week FROM created_at AT TIME ZONE 'America/New_York')) STORED;

* alter table test_table add created_local text GENERATED ALWAYS AS
(EXTRACT(isoyear FROM created_at AT TIME ZONE 'America/New_York')::text ||
'|' || EXTRACT(week FROM created_at AT TIME ZONE 'America/New_York')::text)
STORED;

* CREATE INDEX ON test_table ((EXTRACT(isoyear FROM created_at AT TIME ZONE
'America/New_York') || '|' || EXTRACT(week FROM created_at AT TIME ZONE
'America/New_York')));

However, the following DOES NOT work with an error of (ERROR: generation
expression is not immutable):

* alter table test_table add created_local text GENERATED ALWAYS AS
(EXTRACT(isoyear FROM created_at AT TIME ZONE 'America/New_York') || '|' ||
EXTRACT(week FROM created_at AT TIME ZONE 'America/New_York')) STORED;

Given that casting shouldn't "increase" the immutability of an expression,
and expression indexes need also be immutable afaik, I think that there is a
bug somewhere here?

Thank you,
Jim

On Thursday, September 7, 2023, PG Bug reporting form <
noreply(at)postgresql(dot)org> wrote:

> The following bug has been logged on the website:
>
> Bug reference: 18097
> Logged by: Jim Keener
> Email address: jim(at)jimkeener(dot)com
> PostgreSQL version: 15.0
> Operating system: Linux
> Description:
>
> However, the following DOES NOT work with an error of (ERROR: generation
> expression is not immutable):
>
> * alter table test_table add created_local text GENERATED ALWAYS AS
> (EXTRACT(isoyear FROM created_at AT TIME ZONE 'America/New_York') || '|' ||
> EXTRACT(week FROM created_at AT TIME ZONE 'America/New_York')) STORED;
>
> Given that casting shouldn't "increase" the immutability of an expression,
> and expression indexes need also be immutable afaik, I think that there is
> a
> bug somewhere here?
>

Casting very much can be a non-immutable activity, dates being the prime
example, and I presume going from numeric to text is indeed defined to be
stable hence the error. This is probably due to needing to consult locale
for deciding how to represent the decimal places divider. This is one of
the few places, assuming you write the function to set an environment
fixing locale to some know value like you did with the time zones, where
creating an immutable function around a stable expression makes sense.

David J.

The issue here, though, is that it works as an expression for an index, but doesn't work as a generated column unless I explicitly cast it to text (which should have happened implicitly anyways). (The cast is turning a non-immutable expression to be immutable.)

I'm also able to make generated fields for the individual function calls, but concatenation doesn't work without the explicit cast.

Jim

On September 8, 2023 11:11:42 AM EDT, "David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>On Thursday, September 7, 2023, PG Bug reporting form <
>noreply(at)postgresql(dot)org> wrote:
>
>> The following bug has been logged on the website:
>>
>> Bug reference: 18097
>> Logged by: Jim Keener
>> Email address: jim(at)jimkeener(dot)com
>> PostgreSQL version: 15.0
>> Operating system: Linux
>> Description:
>>
>> However, the following DOES NOT work with an error of (ERROR: generation
>> expression is not immutable):
>>
>> * alter table test_table add created_local text GENERATED ALWAYS AS
>> (EXTRACT(isoyear FROM created_at AT TIME ZONE 'America/New_York') || '|' ||
>> EXTRACT(week FROM created_at AT TIME ZONE 'America/New_York')) STORED;
>>
>> Given that casting shouldn't "increase" the immutability of an expression,
>> and expression indexes need also be immutable afaik, I think that there is
>> a
>> bug somewhere here?
>>
>
>Casting very much can be a non-immutable activity, dates being the prime
>example, and I presume going from numeric to text is indeed defined to be
>stable hence the error. This is probably due to needing to consult locale
>for deciding how to represent the decimal places divider. This is one of
>the few places, assuming you write the function to set an environment
>fixing locale to some know value like you did with the time zones, where
>creating an immutable function around a stable expression makes sense.
>
>David J.

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

James Keener <jim(at)jimkeener(dot)com> writes:
> The issue here, though, is that it works as an expression for an index, but doesn't work as a generated column unless I explicitly cast it to text (which should have happened implicitly anyways). (The cast is turning a non-immutable expression to be immutable.)

The reason that the generated expression fails is that (if you don't
explicitly cast to text) then it relies on anytextcat(anynonarray,text),
which is only stable, and can't be marked any more restrictively because
depending on the type of the non-text argument the corresponding output
function might not be immutable.

But then why doesn't the equivalent index definition spit up?
I found the answer in indexcmds.c's CheckMutability():

/*
* First run the expression through the planner. This has a couple of
* important consequences. First, function default arguments will get
* inserted, which may affect volatility (consider "default now()").
* Second, inline-able functions will get inlined, which may allow us to
* conclude that the function is really less volatile than it's marked. As
* an example, polymorphic functions must be marked with the most volatile
* behavior that they have for any input type, but once we inline the
* function we may be able to conclude that it's not so volatile for the
* particular input type we're dealing with.
*
* We assume here that expression_planner() won't scribble on its input.
*/
expr = expression_planner(expr);

/* Now we can search for non-immutable functions */
return contain_mutable_functions((Node *) expr);

Applying expression_planner() solves the problem because it inlines
anytextcat(anynonarray,text), resolving that the required cast is
numeric->text which is immutable. The code for generated expressions
omits that step and arrives at the less desirable answer. I wonder
where else we have the same issue.

regards, tom lane

[ moving to pgsql-hackers ]

I wrote:
> Applying expression_planner() solves the problem because it inlines
> anytextcat(anynonarray,text), resolving that the required cast is
> numeric->text which is immutable. The code for generated expressions
> omits that step and arrives at the less desirable answer. I wonder
> where else we have the same issue.

After digging around, I could only find one other place where
outside-the-planner code was doing this wrong: AddRelationNewConstraints
can come to the wrong conclusion about whether it's safe to use
missingMode. So here's a patch series to resolve this. I split it
into three parts mostly because 0002 will only go back to v12 where
we added GENERATED, but the missingMode bug exists in v11.

There are a couple of points worth bikeshedding perhaps. I didn't
spend much thought on the wrapper functions' names, but it's surely
true that the semantic difference between contain_mutable_functions
and ContainMutableFunctions is quite un-apparent from those names.
Anybody got a better idea? It also seemed about fifty-fifty whether
to make the wrappers' argument types be Node * or Expr *. I stuck
with Expr * because that's what the predecessor code CheckMutability()
used, but that's not a very strong argument.

BTW, the test function in 0003 might look funny:

CREATE FUNCTION foolme(timestamptz DEFAULT clock_timestamp())
RETURNS timestamptz
IMMUTABLE AS 'select $1' LANGUAGE sql;

but AFAICS it's perfectly legit. The function itself is indeed immutable,
since it's only "select $1"; it's the default argument that's volatile.

I'll add this to the open CF 2023-11, but we really ought to
get it committed before that so we can ship these bug fixes in
November's releases.

regards, tom lane

I wrote:
> After digging around, I could only find one other place where
> outside-the-planner code was doing this wrong: AddRelationNewConstraints
> can come to the wrong conclusion about whether it's safe to use
> missingMode. So here's a patch series to resolve this.

Argh ... I forgot to mention that there's one other place that
this patch series doesn't address, which is that publicationcmds.c's
check_simple_rowfilter_expr() also checks for volatile functions
without having preprocessed the expression. I'm not entirely sure
that there's a reachable problem in the direction of underestimating
the expression's volatility, given that that logic rejects non-builtin
functions entirely: it seems unlikely that any immutable builtin
function would have a volatile default expression. But it definitely
seems possible that there would be a problem in the other direction,
leading to rejecting row filter expressions that we'd like to allow,
much as in bug #18097.

I'm not sure about a good way to resolve this. Simply applying
expression simplification ahead of the check would break the code's
intent of rejecting non-builtin operators, in the case where such
an operator resolves to an inline-able builtin function. I find
the entire design of check_simple_rowfilter_expr pretty questionable
anyway, and there are a bunch of dubious (and undocumented) individual
decisions like allowing whole-row Vars, using FirstNormalObjectId
rather than FirstUnpinnedObjectId as the cutoff, etc. So I'm not
planning to touch that code, but somebody who was paying attention
when it was written might want to take a second look.

regards, tom lane

Hi,

I noticed that the patchset needs a review and decided to take a look.

> There are a couple of points worth bikeshedding perhaps. I didn't
> spend much thought on the wrapper functions' names, but it's surely
> true that the semantic difference between contain_mutable_functions
> and ContainMutableFunctions is quite un-apparent from those names.
> Anybody got a better idea?

Oh no! We encountered one of the most difficult problems in computer
science [1].

ContainMutableFunctionsAfterPerformingPlannersTransformations() would
be somewhat long but semantically correct. It can be shortened to
ContainMutableFunctionsAfterTransformations() or perhaps
TransformedExprContainMutableFunctions(). Personally I don't mind long
names. This being said, ContainMutableFunctions() doesn't disgusts my
sense of beauty too much either. All in all any name will do IMO.
Naturally ContainVolatileFunctions() should be renamed consistently
with ContainMutableFunctions().

I couldn't find anything wrong with 0001..0003. The parches were
tested in several environments and passed `make check-world`. I
suggest merging them.

[1]: https://martinfowler.com/bliki/TwoHardThings.html

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander(at)timescale(dot)com> writes:
>> There are a couple of points worth bikeshedding perhaps. I didn't
>> spend much thought on the wrapper functions' names, but it's surely
>> true that the semantic difference between contain_mutable_functions
>> and ContainMutableFunctions is quite un-apparent from those names.
>> Anybody got a better idea?

> Oh no! We encountered one of the most difficult problems in computer
> science [1].

Indeed :-(. Looking at it again this morning, I'm thinking of
using "contain_mutable_functions_after_planning" --- what do you
think of that?

regards, tom lane

Hi,

> > Oh no! We encountered one of the most difficult problems in computer
> > science [1].
>
> Indeed :-(. Looking at it again this morning, I'm thinking of
> using "contain_mutable_functions_after_planning" --- what do you
> think of that?

It's better but creates an impression that the actual planning will be
involved. According to the comments for expression_planner():

```
* Currently, we disallow sublinks in standalone expressions, so there's no
* real "planning" involved here. (That might not always be true though.)
```

I'm not very well familiar with the part of code responsible for
planning, but I find this inconsistency confusing.

Since the code is written for people to be read and is read more often
than written personally I believe that longer and more descriptive
names are better. Something like
contain_mutable_functions_after_planner_transformations(). This being
said, in practice one should read the comments to learn about corner
cases, pre- and postconditions anyway, so maybe it's not that a big
deal. I think of contain_mutable_functions_after_transformations() as
a good compromise between the length and descriptiveness.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander(at)timescale(dot)com> writes:
>>> Oh no! We encountered one of the most difficult problems in computer
>>> science [1].

>> Indeed :-(. Looking at it again this morning, I'm thinking of
>> using "contain_mutable_functions_after_planning" --- what do you
>> think of that?

> It's better but creates an impression that the actual planning will be
> involved.

True, but from the perspective of the affected code, the question is
basically "did you call expression_planner() yet". So I like this
naming for that connection, whereas something based on "transformation"
doesn't really connect to anything in existing function names.

regards, tom lane

Hi,

> True, but from the perspective of the affected code, the question is
> basically "did you call expression_planner() yet". So I like this
> naming for that connection, whereas something based on "transformation"
> doesn't really connect to anything in existing function names.

Fair enough.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander(at)timescale(dot)com> writes:
>> True, but from the perspective of the affected code, the question is
>> basically "did you call expression_planner() yet". So I like this
>> naming for that connection, whereas something based on "transformation"
>> doesn't really connect to anything in existing function names.

> Fair enough.

Pushed like that, then. Thanks for reviewing!

regards, tom lane

Hello,

A customer encountered an issue while restoring a dump of its database
after applying 15.6 minor version.

It seems due to this fix :

> Fix function volatility checking for GENERATED and DEFAULT
expressions (Tom Lane)
> These places could fail to detect insertion of a volatile function
default-argument expression, or decide that a polymorphic function is
volatile although it is actually immutable on the datatype of interest.
This could lead to improperly rejecting or accepting a GENERATED clause,
or to mistakenly applying the constant-default-value optimization in
ALTER TABLE ADD COLUMN.

Related commit 9057ddbef

I managed to reproduce it with a simple test case :

CREATE SCHEMA s1;
CREATE SCHEMA s2;

CREATE FUNCTION s2.f1 (c1 text) RETURNS text
LANGUAGE SQL IMMUTABLE
AS $$
SELECT c1
$$;

CREATE FUNCTION s2.f2 (c1 text) RETURNS text
LANGUAGE SQL IMMUTABLE
AS $$
SELECT s2.f1 (c1);
$$;

CREATE TABLE s1.t1 (c1 text, c2 text GENERATED ALWAYS AS (s2.f2 (c1))
STORED);

CREATE FUNCTION s1.f3 () RETURNS SETOF s1.t1
LANGUAGE sql
AS $$
SELECT *
FROM s1.t1
$$;

The resulting dump is attached.

You will notice that the table s1.t1 is created before the function
s2.f1. This is due to the function s1.f3 which returns a SETOF s1.t1

I understand Postgres has to create s1.t1 before s1.f3. Unfortunately,
the function s2.f1 is created later.

When we try to restore the dump, we have this error :
CREATE TABLE s1.t1 (
c1 text,
c2 text GENERATED ALWAYS AS (s2.f2(c1)) STORED
);
psql:b2.sql:61: ERROR: function s2.f1(text) does not exist
LINE 2: SELECT s2.f1(c1);
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.
QUERY:
SELECT s2.f1(c1);

CONTEXT: SQL function "f2" during inlining

Thanks to Jordi Morillo, Alexis Lucazeau, Matthieu Honel for reporting this.

Regards,

--
Adrien NAYRAT

Adrien Nayrat <adrien(dot)nayrat(at)anayrat(dot)info> writes:
> A customer encountered an issue while restoring a dump of its database
> after applying 15.6 minor version.
> It seems due to this fix :
>>> Fix function volatility checking for GENERATED and DEFAULT
>>> expressions (Tom Lane)

I don't believe this example has anything to do with that.

> CREATE SCHEMA s1;
> CREATE SCHEMA s2;
> CREATE FUNCTION s2.f1 (c1 text) RETURNS text
> LANGUAGE SQL IMMUTABLE
> AS $$
> SELECT c1
> $$;
> CREATE FUNCTION s2.f2 (c1 text) RETURNS text
> LANGUAGE SQL IMMUTABLE
> AS $$
> SELECT s2.f1 (c1);
> $$;
> CREATE TABLE s1.t1 (c1 text, c2 text GENERATED ALWAYS AS (s2.f2 (c1))
> STORED);

The problem here is that to pg_dump, the body of s2.f2 is just an
opaque string, so it has no idea that that depends on s2.f1, and
it ends up picking a dump order that doesn't respect that
dependency.

It used to be that there wasn't much you could do about this
except choose object names that wouldn't cause the problem.
In v14 and up there's another way, at least for SQL-language
functions: you can write the function in SQL spec style.

CREATE FUNCTION s2.f2 (c1 text) RETURNS text
IMMUTABLE
BEGIN ATOMIC
SELECT s2.f1 (c1);
END;

Then the dependency is visible, both to the server and to pg_dump,
and you get a valid dump order.

regards, tom lane

On 9/25/24 4:41 PM, Tom Lane wrote:
> Adrien Nayrat <adrien(dot)nayrat(at)anayrat(dot)info> writes:
>> A customer encountered an issue while restoring a dump of its database
>> after applying 15.6 minor version.
>> It seems due to this fix :
>>>> Fix function volatility checking for GENERATED and DEFAULT
>>>> expressions (Tom Lane)
>
> I don't believe this example has anything to do with that.

I've done a git bisect between 15.5 and 15.6 and this commit trigger the
error.

>
>> CREATE SCHEMA s1;
>> CREATE SCHEMA s2;
>> CREATE FUNCTION s2.f1 (c1 text) RETURNS text
>> LANGUAGE SQL IMMUTABLE
>> AS $$
>> SELECT c1
>> $$;
>> CREATE FUNCTION s2.f2 (c1 text) RETURNS text
>> LANGUAGE SQL IMMUTABLE
>> AS $$
>> SELECT s2.f1 (c1);
>> $$;
>> CREATE TABLE s1.t1 (c1 text, c2 text GENERATED ALWAYS AS (s2.f2 (c1))
>> STORED);
>
> The problem here is that to pg_dump, the body of s2.f2 is just an
> opaque string, so it has no idea that that depends on s2.f1, and
> it ends up picking a dump order that doesn't respect that
> dependency.
>
> It used to be that there wasn't much you could do about this
> except choose object names that wouldn't cause the problem.

I see. So I understand we were lucky it worked before the commit added
the check of volatility in generated column ?

> In v14 and up there's another way, at least for SQL-language
> functions: you can write the function in SQL spec style.
>
> CREATE FUNCTION s2.f2 (c1 text) RETURNS text
> IMMUTABLE
> BEGIN ATOMIC
> SELECT s2.f1 (c1);
> END;
>
> Then the dependency is visible, both to the server and to pg_dump,
> and you get a valid dump order.
>

Oh, thanks !

--
Adrien NAYRAT
https://pro.anayrat.info

Adrien Nayrat <adrien(dot)nayrat(at)anayrat(dot)info> writes:
> I see. So I understand we were lucky it worked before the commit added
> the check of volatility in generated column ?

Pretty much. There are other cases that could trigger expansion
of such a function before the restore is complete. It is unfortunate
that this bit you in a minor release, but there are lots of other
ways you could have tripped over the missing dependency unexpectedly.

regards, tom lane