Tom, all, * Tom Lane (t...@sss.pgh.pa.us) wrote: > =?iso-8859-1?Q?Bo_Thorbj=F8rn_Jensen?= <b...@budget123.dk> writes: > > I have some additional info and a fix. > > Firstly steps to reproduce: > > Yeah, I can reproduce this. I suspect it got broken by Stephen's hacking > around with default ACLs. A simple example is
Yes, it's related to the work I did with pg_dump's ACL handling, because we're no longer just always including the whole revoke/grant set of ACLs for everything in the output. > $ pg_dump -c -U postgres postgres | grep -i public > DROP SCHEMA public; > -- Name: public; Type: SCHEMA; Schema: -; Owner: postgres > CREATE SCHEMA public; > ALTER SCHEMA public OWNER TO postgres; > -- Name: SCHEMA public; Type: COMMENT; Schema: -; Owner: postgres > COMMENT ON SCHEMA public IS 'standard public schema'; > -- Name: public; Type: ACL; Schema: -; Owner: postgres > GRANT ALL ON SCHEMA public TO PUBLIC; > > That's fine, but if I shove it through an archive file: This works because I added into pg_dump.c a check based on if the output is clean (and therefore the public schema is being recreated or not). In hindsight, that wasn't really the right thing to do because it ends up only working when pg_dump is run with -c and doesn't consider the case where pg_dump is run without -c but pg_restore is. > $ pg_dump -f p.dump -Fc -U postgres postgres > > $ pg_restore -c p.dump | grep -i public This doesn't work because pg_dump isn't run with -c, while pg_restore is. If the archive is created with pg_dump -c (as the above was), then the results match up between the two runs. Note also that if pg_dump is run with -c then a pg_restore without -c would actually still include the GRANT statement, which isn't really correct either. That's obviously a change from what we had before and wasn't intentional. > This is *REALLY BAD*. Quite aside from the restore being wrong, > those two sequences should never ever give different results. > Stephen, you put some filtering logic in the wrong place in pg_dump. I do wish it was that simple. Unfortunately, the public schema is just ridiculously special, both in the way it's a 'user' object but is created by initdb and that it's got special non-default ACLs on it and how it has explicit special code to skip over it when a restore is happening, unless -c is used. What I'm afraid we need to do here is basically continue to hack on that code in pg_backup_archiver.c's _printTocEntry() to teach it to issue the default GRANT ALL ON SCHEMA public TO PUBLIC; when we are processing the TOC entry for CREATE SCHEMA public;. That would make the recreation of the public schema when pg_dump or pg_restore is being run with -c actually match how the public schema is created by initdb, and the rest would end up falling into place, I think. One complication, however, is what happens when a user drops and recreates the public schema. If that's done, we'll end up not dumping out the delta from the public schema's initial ACLs, which wouldn't be correct if you're restoring into a newly initdb'd cluster. I'm thinking that we need to forcibly look at the delta from public-as-installed-by-initdb and whatever-public-is-now, regardless of if the public schema was recreated by the user or not, because on restore we are expecting a newly initdb'd cluster with the public schema as originally installed (or as installed by pg_dump/pg_restore following the logic above). I'll play around with this approach and see if things end up working out in a better fashion with it. Baking this knowledge into pg_backup_archiver.c is certainly ugly, but handling of public has always been hard-coded into that, and we even added more special handling to that code 10 years ago to deal with the COMMENT on the public schema, so this is really just more of the same. Thanks! Stephen
signature.asc
Description: Digital signature
