Re: [PATCH] minor bug fix for pg_dump --clean - Mailing list pgsql-hackers
From | Frédéric Yhuel |
---|---|
Subject | Re: [PATCH] minor bug fix for pg_dump --clean |
Date | |
Msg-id | d146cbc4-6daf-5ee1-519f-208afa3c8ccc@dalibo.com Whole thread Raw |
In response to | Re: [PATCH] minor bug fix for pg_dump --clean (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: [PATCH] minor bug fix for pg_dump --clean
(Bruce Momjian <bruce@momjian.us>)
|
List | pgsql-hackers |
On 10/24/22 03:01, Tom Lane wrote: > =?UTF-8?Q?Fr=c3=a9d=c3=a9ric_Yhuel?= <frederic.yhuel@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
pgsql-hackers by date: