Обсуждение: BUG #18151: pg_upgradecluster fails when column default refers to column

The following bug has been logged on the website:

Bug reference:      18151
Logged by:          Liam Morland
Email address:      liam@morland.ca
PostgreSQL version: 15.4
Operating system:   Debian
Description:

I was trying to upgrade to PG 16:
pg_upgradecluster 15 main

I got this error:
pg_restore: error: could not execute query: ERROR:  relation "event" does
not exist

The reason is the the default value for column "event_id" of table "event"
is "event_id_nextval()". That function is:
SELECT COALESCE(MAX(event_id) + 1, 1) FROM event;

I suppose that what is happening is a circular dependency, the table "event"
does not exist yet, because it is in the process of restoring it. I was able
to clear the error by setting the column default to NULL, running
pg_upgradecluster, and restoring the column default back to
"event_id_nextval()".

I did not have to do this on previous upgrades, so something has changed. In
the past, it just worked.

I do not understand why it would have to execute "event_id_nextval()" to do
the restore. All the rows already have a value in that column (they have to,
it is the primary key). (Yes, I am aware of SERIAL.)

A related issue: It would have been better if pg_upgradecluster had not
output a success message and switched to the new cluster. It should have
detected the error, output an error message, and left the old cluster
running.

PG Bug reporting form <noreply@postgresql.org> writes:
> I was trying to upgrade to PG 16:
> pg_upgradecluster 15 main

> I got this error:
> pg_restore: error: could not execute query: ERROR:  relation "event" does
> not exist

> The reason is the the default value for column "event_id" of table "event"
> is "event_id_nextval()". That function is:
> SELECT COALESCE(MAX(event_id) + 1, 1) FROM event;

Thanks for the report!

> I do not understand why it would have to execute "event_id_nextval()" to do
> the restore.

It shouldn't.  I think this might be a variant of the bug recently
reported here:

https://www.postgresql.org/message-id/flat/75a7b7483aeb331aa017328d606d568fc715b90d.camel%40cybertec.at

However, that doesn't seem quite right because that'd result in a useless
extra evaluation during COPY to the table, at which time the table
surely must exist.  Also, pg_upgrade shouldn't need to use COPY at all.

Is that function written in old-style (with a string literal for the
body) or new-style with BEGIN ATOMIC?  In the latter case it's possible
that you've got a circular dependency that pg_dump is failing to work
around.

Can you show us the exact DDL definition of both the table and the
function?

> A related issue: It would have been better if pg_upgradecluster had not
> output a success message and switched to the new cluster. It should have
> detected the error, output an error message, and left the old cluster
> running.

As far as that goes, you'd have to complain to the Debian maintainers
of pg_upgradecluster.  That code doesn't belong to the core project.

            regards, tom lane

The following bug has been logged on the website:

Bug reference:      18151
Logged by:          Liam Morland
Email address:      liam@morland.ca
PostgreSQL version: 15.4
Operating system:   Debian
Description:       

I was trying to upgrade to PG 16:
pg_upgradecluster 15 main

I got this error:
pg_restore: error: could not execute query: ERROR:  relation "event" does
not exist

The reason is the the default value for column "event_id" of table "event"
is "event_id_nextval()". That function is:
SELECT COALESCE(MAX(event_id) + 1, 1) FROM event;

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Sunday, October 8, 2023, PG Bug reporting form <noreply@postgresql.org>
> wrote:
>> The reason is the the default value for column "event_id" of table "event"
>> is "event_id_nextval()". That function is:
>> SELECT COALESCE(MAX(event_id) + 1, 1) FROM event;

> You are expressly forbidden to have the default value expression of a
> column reference a subquery. You must use a trigger.

Well, you're not allowed to do it directly:

regression=# create table foo (f1 int default ((select max(f1) from foo)));
ERROR:  cannot use subquery in DEFAULT expression
LINE 1: create table foo (f1 int default ((select max(f1) from foo))...
                                         ^

but that's just a minor implementation restriction.  Doing it through
a function is fine.  (Whether it's a good idea is another question.)
Unlike, say, CHECK constraints, there's not any expectation that
a default expression be immutable or avoid dependence on database
state --- if there was, "default nextval(...)" would be problematic.

In any case, dump/restore ought not fail like this.

            regards, tom lane

2023-10-09 10:11-0400 Tom Lane <tgl@sss.pgh.pa.us> wrote: [...]
>Is that function written in old-style (with a string literal for the 
>body) or new-style with BEGIN ATOMIC? [...]

It is old-style. I wrote it before PG 14.

>Can you show us the exact DDL definition of both the table and the 
>function? [...]

From the output of pg_dump:

CREATE FUNCTION public.event_id_nextval() RETURNS integer
    LANGUAGE sql
    AS $$
  SELECT COALESCE(MAX(event_id) + 1, 1) FROM event;
  $$;

CREATE TABLE public.event (
    event_id integer DEFAULT public.event_id_nextval() NOT NULL,
    date_start date NOT NULL,
    date_end date NOT NULL,
    title text NOT NULL,
    CONSTRAINT date_end_gt_date_start CHECK ((date_end >= date_start))
);

ALTER TABLE ONLY public.event
    ADD CONSTRAINT event_pkey PRIMARY KEY (event_id);

Regards,
Liam

Liam Morland <liam@morland.ca> writes:
> 2023-10-09 10:11-0400 Tom Lane <tgl@sss.pgh.pa.us> wrote: [...]
>> Is that function written in old-style (with a string literal for the 
>> body) or new-style with BEGIN ATOMIC? [...]

> It is old-style. I wrote it before PG 14.

OK, so it should work ...

>> Can you show us the exact DDL definition of both the table and the 
>> function? [...]

>> From the output of pg_dump:

> CREATE FUNCTION public.event_id_nextval() RETURNS integer
>     LANGUAGE sql
>     AS $$
>   SELECT COALESCE(MAX(event_id) + 1, 1) FROM event;
>   $$;

> CREATE TABLE public.event (
>     event_id integer DEFAULT public.event_id_nextval() NOT NULL,


Oh ... I thought of a plausible explanation, or part of an
explanation.  That function is not search-path-safe: if it's run with
a search_path that doesn't include "public", it'll fail as described.
And indeed dump/restore will use a restrictive search_path setting.
So the COPY bug I alluded to before could trigger the reported
failure, if the upgrade is transferring data to the new cluster using
COPY rather than physically moving files around.

I see that pg_upgradecluster defaults to using dump/restore rather
than pg_upgrade, which surprises me, but if you used that mode then
all is explained.

Bug or no bug, that function would be better written as

   SELECT COALESCE(MAX(event_id) + 1, 1) FROM public.event;

so that it still works under a restrictive search path.

            regards, tom lane