Обсуждение: [GENERAL] missing public on schema public

Bo Thorbjørn Jensen <bo@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

$ 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:

$ pg_dump -f p.dump -Fc -U postgres postgres

$ pg_restore -c p.dump | 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';

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.
        regards, tom lane


-- 
Sent via pgsql-general mailing list (pgsql-general@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Bo Thorbjørn Jensen <bo@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

Was this ever fixed?


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Mar 26, 2018 at 12:46:38PM -0300, Alvaro Herrera wrote:
> Was this ever fixed?

Ugh.  I have added a reminder on the open item page for v11 as an older
bug:
https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Older_Bugs
--
Michael

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 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.

I see that this is listed on the open items for v11, but it's hard to
justify it being there, because the bug exists only in 9.6 and 10.
(We fixed it in HEAD as part of the pg_dump-vs-pg_dumpall refactoring.)

I gather, from the lack of any progress since November, that you're
probably not going to fix it in the back branches.  I'm not excited
about working on it either, but I dislike leaving such a bug unfixed.

In any case, I think it should be removed from the v11 list.  The "older
bugs" section is meant to capture pre-existing bugs that we might possibly
fix as part of v11 stabilization, and this isn't that.

            regards, tom lane