Re: [PATCH] minor bug fix for pg_dump --clean

On 10/24/22 03:01, Tom Lane wrote:
> =?UTF-8?Q?Fr=c3=a9d=c3=a9ric_Yhuel?= <frederic(dot)yhuel(at)dalibo(dot)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