Thread: BUG #8695: Reloading dump fails at COMMENT ON EXTENSION plpgsql
The following bug has been logged on the website: Bug reference: 8695 Logged by: Christian Ullrich Email address: chris@chrullrich.net PostgreSQL version: 9.3.2 Operating system: all Description: A non-superuser cannot reload any dump of a database that contains the plpgsql extension, because the dump unconditionally attempts to set the comment on that extension. This fails because plpgsql is owned by the superuser who installed it. This contradicts the manual, which says: "The dumps produced by pg_dump are relative to template0." The plpgsql extension is present in template0, with the identical comment, and therefore neither extension nor comment should be dumped at all. (I know this is splitting hairs, because pg_dump does not actually compare the subject database to template0, but still, the contradiction is there.) The extension itself is dumped as CREATE IF NOT EXISTS, so that works, but there is no conditional syntax for comments, and since pg_dump does not know whether the comment has been changed from the default, it could not use one anyway. I can think of one possible fix (aside from simply filtering that line from the dump): COMMENT could be a no-op if the requested comment is identical to the existing one. Another idea I had was to allow comments to be part of an extension, so that pg_dump would not dump them, but that does not work because pg_dump does not know if a comment has been changed from the original value. Not that anyone would ever do that.
On Sun, Dec 22, 2013 at 01:56:13AM +0000, chris@chrullrich.net wrote:
> The following bug has been logged on the website:
>
> Bug reference: 8695
> Logged by: Christian Ullrich
> Email address: chris@chrullrich.net
> PostgreSQL version: 9.3.2
> Operating system: all
> Description:
>
> A non-superuser cannot reload any dump of a database that contains the
> plpgsql extension, because the dump unconditionally attempts to set the
> comment on that extension. This fails because plpgsql is owned by the
> superuser who installed it.
>
>
> This contradicts the manual, which says: "The dumps produced by pg_dump are
> relative to template0." The plpgsql extension is present in template0, with
> the identical comment, and therefore neither extension nor comment should be
> dumped at all. (I know this is splitting hairs, because pg_dump does not
> actually compare the subject database to template0, but still, the
> contradiction is there.)
>
>
> The extension itself is dumped as CREATE IF NOT EXISTS, so that works, but
> there is no conditional syntax for comments, and since pg_dump does not know
> whether the comment has been changed from the default, it could not use one
> anyway.
>
>
> I can think of one possible fix (aside from simply filtering that line from
> the dump): COMMENT could be a no-op if the requested comment is identical to
> the existing one.
>
>
> Another idea I had was to allow comments to be part of an extension, so that
> pg_dump would not dump them, but that does not work because pg_dump does not
> know if a comment has been changed from the original value. Not that anyone
> would ever do that.
I can reproduce this bug:
$ psql test
psql (9.4devel)
Type "help" for help.
test=> CREATE USER joe;
CREATE ROLE
test=> CREATE DATABASE test2 OWNER joe;
CREATE DATABASE
test=> \q
$ pg_dump test | psql -e -U joe test2
SET statement_timeout = 0;
SET
SET lock_timeout = 0;
SET
SET client_encoding = 'UTF8';
SET
SET standard_conforming_strings = on;
SET
SET check_function_bodies = false;
SET
SET client_min_messages = warning;
SET
CREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog;
CREATE EXTENSION
--> COMMENT ON EXTENSION plpgsql IS 'PL/pgSQL procedural language';
--> ERROR: must be owner of extension plpgsql
REVOKE ALL ON SCHEMA public FROM PUBLIC;
WARNING: no privileges could be revoked for "public"
REVOKE
REVOKE ALL ON SCHEMA public FROM postgres;
WARNING: no privileges could be revoked for "public"
REVOKE
GRANT ALL ON SCHEMA public TO postgres;
WARNING: no privileges were granted for "public"
GRANT
GRANT ALL ON SCHEMA public TO PUBLIC;
WARNING: no privileges were granted for "public"
GRANT
This would certainly cause a restore to abort for a non-super-user if
psql used --set ON_ERROR_STOP=on. Any easy way to fix this? I am not
super-excited about the suggested fixes listed above.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
* Bruce Momjian wrote: > On Sun, Dec 22, 2013 at 01:56:13AM +0000, chris@chrullrich.net wrote: >> A non-superuser cannot reload any dump of a database that contains the >> plpgsql extension, because the dump unconditionally attempts to set the >> comment on that extension. This fails because plpgsql is owned by the >> superuser who installed it. >> I can think of one possible fix (aside from simply filtering that line from >> the dump): COMMENT could be a no-op if the requested comment is identical to >> the existing one. >> >> Another idea I had was to allow comments to be part of an extension, so that >> pg_dump would not dump them, but that does not work because pg_dump does not >> know if a comment has been changed from the original value. Not that anyone >> would ever do that. > This would certainly cause a restore to abort for a non-super-user if > psql used --set ON_ERROR_STOP=on. Any easy way to fix this? I am not > super-excited about the suggested fixes listed above. Not just with ON_ERROR_STOP, but also with --single-transaction. IMO, that is even worse, because restoring that way is a) a lot faster for non-enormous databases, and b) easier to clean up if some other error happens. Warning: Raw brain dump following. As for possible fixes, you can find an argument in favor of my no-op suggestion below, along with some more ideas that probably won't work, but the root cause is the "relative to template0" thing. As long as pg_dump does not adhere to that, the problem remains. It is impossible to make it adhere without expending large amounts of work both on development and at runtime, so that statement should at least be changed to mention that comments on extensions are included in the dump. Other than that, I think my suggestion of letting COMMENT succeed (fail silently, actually) if the comment would not change even if the command were executed is not that bad. If the command is -- in the specific situation -- idempotent, why not simply skip it? (The only situation where that might be a problem is if someone made pg_description not readable to everyone, thereby breaking all the \d..+ psql commands for nonprivileged users. In that case, such a user could possibly probe for existing comments, whatever good that would do them. It's at least imaginable that some application might abuse the comments for configuration metadata. Of course, COMMENT would have to read the existing comment, thereby failing early without leaking.) The simplest fix I can think of on further consideration is to give the database owner blanket COMMENT privilege on everything. Without checking the archives, I suspect that idea was considered and rejected when comments were introduced, because it looks rather obvious. Another idea: Per-user comments. If the user invoking COMMENT does not have the required privileges to set the global comment, a per-user comment would be set instead, closely followed by chaos and confusion. Yet another: COMMENT IF NOT EXISTS, without looking at the existing comment. Nicely self-contained, but the semantics if there is no comment yet look questionable: Loading a dump with a comment for an existing object that does not have one in the target database would fail again, just as it does now. That might be considered correct because we're trying to set a comment where none was before, but it does not pass the "relative to template0" test either if the comment existed in there and was removed in the dumped database. And one more: Extension ownership could implicitly follow database ownership. Not backward compatible; would give database owners additional privileges simply by upgrading, and would obsolete the pg_extension.extowner column. -- Christian
On Mon, Mar 31, 2014 at 01:00:09PM -0400, Bruce Momjian wrote:
> CREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog;
> CREATE EXTENSION
> --> COMMENT ON EXTENSION plpgsql IS 'PL/pgSQL procedural language';
> --> ERROR: must be owner of extension plpgsql
> REVOKE ALL ON SCHEMA public FROM PUBLIC;
> WARNING: no privileges could be revoked for "public"
> REVOKE
> REVOKE ALL ON SCHEMA public FROM postgres;
> WARNING: no privileges could be revoked for "public"
> REVOKE
> GRANT ALL ON SCHEMA public TO postgres;
> WARNING: no privileges were granted for "public"
> GRANT
> GRANT ALL ON SCHEMA public TO PUBLIC;
> WARNING: no privileges were granted for "public"
> GRANT
>
> This would certainly cause a restore to abort for a non-super-user if
> psql used --set ON_ERROR_STOP=on. Any easy way to fix this? I am not
> super-excited about the suggested fixes listed above.
In looking at this further, I wonder why we don't filter out pg_catalog
extensions when we dump them; right now we use:
appendPQExpBufferStr(query, "SELECT x.tableoid, x.oid, "
"x.extname, n.nspname, x.extrelocatable, x.extversion, x.extconfig, x.extcondition "
"FROM pg_extension x "
"JOIN pg_namespace n ON n.oid = x.extnamespace");
I see a filter on pg_catalog for pg_proc. Is there a reason there isn't
one for pg_extension?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> I see a filter on pg_catalog for pg_proc. Is there a reason there isn't
> one for pg_extension?
Extensions don't live in schemas.
regards, tom lane
On Wed, Apr 9, 2014 at 10:15:28PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I see a filter on pg_catalog for pg_proc. Is there a reason there isn't
> > one for pg_extension?
>
> Extensions don't live in schemas.
That's what I thought too, but I see a schema file in pg_extensions:
test=> \d pg_extension
Table "pg_catalog.pg_extension"
Column | Type | Modifiers
----------------+---------+-----------
extname | name | not null
extowner | oid | not null
--> extnamespace | oid | not null
extrelocatable | boolean | not null
extversion | text |
extconfig | oid[] |
extcondition | text[] |
Indexes:
"pg_extension_name_index" UNIQUE, btree (extname)
"pg_extension_oid_index" UNIQUE, btree (oid)
SELECT extname, extnamespace, nspname
FROM pg_extension, pg_namespace
WHERE extnamespace = pg_namespace.oid;
extname | extnamespace | nspname
---------+--------------+------------
plpgsql | 11 | pg_catalog
(1 row)
Should we hard-code a pg_catalog plpgsql to be skipped in pg_dump?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> That's what I thought too, but I see a schema file in pg_extensions:
Read the manual.
> Should we hard-code a pg_catalog plpgsql to be skipped in pg_dump?
No, I don't think so.
The real issue here is that we don't have a notion of a "built-in
extension". I think this was specifically debated back when we
extension-ified plpgsql, though I don't recall details of why
we ended up not doing that. Maybe the idea was that you could
drop and then re-add plpgsql? Anyway, I think this is not such
a simple issue and a one-line hack in pg_dump is not likely to
improve matters.
regards, tom lane
On Wed, Apr 9, 2014 at 11:13:57PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > That's what I thought too, but I see a schema file in pg_extensions:
>
> Read the manual.
>
> > Should we hard-code a pg_catalog plpgsql to be skipped in pg_dump?
>
> No, I don't think so.
>
> The real issue here is that we don't have a notion of a "built-in
> extension". I think this was specifically debated back when we
> extension-ified plpgsql, though I don't recall details of why
> we ended up not doing that. Maybe the idea was that you could
> drop and then re-add plpgsql? Anyway, I think this is not such
> a simple issue and a one-line hack in pg_dump is not likely to
> improve matters.
OK, I added a TODO:
Prevent PL/pgSQL comment from throwing an error in a
non-superuser restore
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +