On 10/24/22 03:01, Tom Lane wrote:
=?UTF-8?Q?Fr=c3=a9d=c3=a9ric_Yhuel?= <frederic.yh...@dalibo.com> writes:
When using pg_dump (or pg_restore) with option "--clean", there is some
SQL code to drop every objects at the beginning.
Yup ...
The DROP statement for a view involving circular dependencies is :
CREATE OR REPLACE VIEW [...]
(see commit message of d8c05aff for a much better explanation)
If the view is not in the "public" schema, and the target database is
empty, this statement fails, because the schema hasn't been created yet.
The attached patches are a TAP test which can be used to reproduce the
bug, and a proposed fix. They apply to the master branch.
I am disinclined to accept this as a valid bug, because there's never
been any guarantee that a --clean script would execute error-free in
a database that doesn't match what the source database contains.
(The pg_dump documentation used to say that in so many words.
I see that whoever added the --if-exists option was under the
fond illusion that that fixes all cases, which it surely does not.
We need to back off the promises a bit there.)
An example of a case that won't execute error-free is if the view
having a circular dependency includes a column of a non-built-in
data type. If you try to run that in an empty database, you'll
get an error from the CREATE OR REPLACE VIEW's reference to that
data type. For instance, if I adjust your test case to make
the "payload" column be of type hstore, I get something like
psql:dumpresult.sql:22: ERROR: type "public.hstore" does not exist
LINE 4: NULL::public.hstore AS payload;
^
The same type of failure occurs for user-defined functions and
operators that use a non-built-in type, and I'm sure there are
more cases in the same vein. But it gets *really* messy if
the target database isn't completely empty, but contains objects
with different properties than the dump script expects; for example,
if the view being discussed here exists with a different column set
than the script thinks, or if the dependency chains aren't all the
same.
If this fix were cleaner I might be inclined to accept it anyway,
but it's not very clean at all --- for example, it's far from
obvious to me what are the side-effects of changing the filter
in RestoreArchive like that. Nor am I sure that the schema
you want to create is guaranteed to get dropped again later in
every use-case.
Hi Tom, Viktoria,
Thank you for your review Viktoria!
Thank you for this detailed explanation, Tom! I didn't have great hope
for this patch. I thought that the TAP test could be accepted, but now I
can see that it is clearly useless.
So I think mainly what we ought to do here is to adjust the
documentation to make it clearer that --clean is not guaranteed
to work without errors unless the target database has the same
set of objects as the source. --if-exists can reduce the set
of error cases, but not eliminate it. Possibly we should be
more enthusiastic about recommending --create --clean (ie,
drop and recreate the whole database) instead.
I beleive a documentation patch would be useful, indeed.
Best regards,
Frédéric
