Обсуждение: Re: [COMMITTERS] pgsql: Fix pg_dump to dump shell types.

Andrew Dunstan <andrew@dunslane.net> writes:
> Still not quite there. If either 9.0 or 9.1 is upgraded to 9.2 or later, 
> they fail like this:

>     pg_restore: creating TYPE "public"."myshell"
>     pg_restore: setting owner and privileges for TYPE "public"."myshell"
>     pg_restore: setting owner and privileges for ACL "public"."myshell"
>     pg_restore: [archiver (db)] Error from TOC entry 4293; 0 0 ACL
>     myshell buildfarm
>     pg_restore: [archiver (db)] could not execute query: ERROR:  type
>     "myshell" is only a shell
>          Command was: REVOKE ALL ON TYPE "myshell" FROM PUBLIC;
>     REVOKE ALL ON TYPE "myshell" FROM "buildfarm";
>     GRANT ALL ON TYPE "myshell" TO PUBL...

> We could just declare that we don't support this for versions older than 
> 9.2, in which case I would just remove the type from the test database 
> before testing cross-version upgrade. But if it's easily fixed then 
> let's do it.

It looks to me like the reason for this is that pg_dump forces the
"typacl" of a type to be '{=U}' when reading the schema data for a
pre-9.2 type, rather than reading it as NULL (ie default permissions)
which would result in not printing any grant/revoke commands for
the object.

I do not see a good reason for that; quite aside from this problem,
it means there is one more place that knows the default permissions
for a type than there needs to be.  Peter, what was the rationale?
        regards, tom lane

On 8/9/15 6:23 PM, Tom Lane wrote:
> It looks to me like the reason for this is that pg_dump forces the
> "typacl" of a type to be '{=U}' when reading the schema data for a
> pre-9.2 type, rather than reading it as NULL (ie default permissions)
> which would result in not printing any grant/revoke commands for
> the object.
> 
> I do not see a good reason for that; quite aside from this problem,
> it means there is one more place that knows the default permissions
> for a type than there needs to be.  Peter, what was the rationale?

This was probably just copied from how proacl and lanacl are handled,
which predate typacl by quite a bit.  Maybe there was a reason in those
days.

It might also have something to do with how owner privileges are
handled.  An explicit '{=U}' doesn't create owner privileges, unlike a
null value in that field.  Maybe this is necessary if you dump and
restore between databases with different user names.

Peter Eisentraut <peter_e@gmx.net> writes:
> On 8/9/15 6:23 PM, Tom Lane wrote:
>> It looks to me like the reason for this is that pg_dump forces the
>> "typacl" of a type to be '{=U}' when reading the schema data for a
>> pre-9.2 type, rather than reading it as NULL (ie default permissions)
>> which would result in not printing any grant/revoke commands for
>> the object.
>> 
>> I do not see a good reason for that; quite aside from this problem,
>> it means there is one more place that knows the default permissions
>> for a type than there needs to be.  Peter, what was the rationale?

> This was probably just copied from how proacl and lanacl are handled,
> which predate typacl by quite a bit.  Maybe there was a reason in those
> days.

Hm ... I wonder whether those are well-thought-out either.

> It might also have something to do with how owner privileges are
> handled.  An explicit '{=U}' doesn't create owner privileges, unlike a
> null value in that field.  Maybe this is necessary if you dump and
> restore between databases with different user names.

But now that you mention it, isn't that completely broken?  What pg_dump
actually prints given this made-up data is

REVOKE ALL ON TYPE myshell FROM PUBLIC;
REVOKE ALL ON TYPE myshell FROM postgres;
GRANT ALL ON TYPE myshell TO PUBLIC;

which seems like a completely insane interpretation.  There is no way
that dumping a type from a pre-typacl database and restoring it into
a newer one should end up with the type's owner having no privileges
on it.  I'm astonished that we've not gotten bug reports about that.
        regards, tom lane

I wrote:
> But now that you mention it, isn't that completely broken?  What pg_dump
> actually prints given this made-up data is

> REVOKE ALL ON TYPE myshell FROM PUBLIC;
> REVOKE ALL ON TYPE myshell FROM postgres;
> GRANT ALL ON TYPE myshell TO PUBLIC;

> which seems like a completely insane interpretation.  There is no way
> that dumping a type from a pre-typacl database and restoring it into
> a newer one should end up with the type's owner having no privileges
> on it.  I'm astonished that we've not gotten bug reports about that.

... of course, the reason for no field reports is that the owner still
has privileges, as she inherits them from PUBLIC.  Doesn't make this
any less broken though.
        regards, tom lane

I wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
>> This was probably just copied from how proacl and lanacl are handled,
>> which predate typacl by quite a bit.  Maybe there was a reason in those
>> days.

> Hm ... I wonder whether those are well-thought-out either.

They're not.  Testing with ancient servers shows that we dump very silly
grant/revoke state for functions and languages as well, if the source
server is too old to have proacl or lanacl (ie, pre-7.3).  As with typacl,
the silliness is accidentally masked as long as the owner doesn't do
something like revoke the privileges granted to PUBLIC.

Things work far more sanely with the attached patch, to wit we just leave
all object privileges as default if dumping from a version too old to have
privileges on that type of object.  I think we should back-patch this into
all supported branches; it's considerably more likely that older branches
would be used to dump from ancient servers.

            regards, tom lane

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 51b6d3a..87dadbf 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** getTypes(Archive *fout, int *numTypes)
*** 3513,3519 ****
      else if (fout->remoteVersion >= 80300)
      {
          appendPQExpBuffer(query, "SELECT tableoid, oid, typname, "
!                           "typnamespace, '{=U}' AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
--- 3513,3519 ----
      else if (fout->remoteVersion >= 80300)
      {
          appendPQExpBuffer(query, "SELECT tableoid, oid, typname, "
!                           "typnamespace, NULL AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
*************** getTypes(Archive *fout, int *numTypes)
*** 3528,3534 ****
      else if (fout->remoteVersion >= 70300)
      {
          appendPQExpBuffer(query, "SELECT tableoid, oid, typname, "
!                           "typnamespace, '{=U}' AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
--- 3528,3534 ----
      else if (fout->remoteVersion >= 70300)
      {
          appendPQExpBuffer(query, "SELECT tableoid, oid, typname, "
!                           "typnamespace, NULL AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
*************** getTypes(Archive *fout, int *numTypes)
*** 3542,3548 ****
      else if (fout->remoteVersion >= 70100)
      {
          appendPQExpBuffer(query, "SELECT tableoid, oid, typname, "
!                           "0::oid AS typnamespace, '{=U}' AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
--- 3542,3548 ----
      else if (fout->remoteVersion >= 70100)
      {
          appendPQExpBuffer(query, "SELECT tableoid, oid, typname, "
!                           "0::oid AS typnamespace, NULL AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
*************** getTypes(Archive *fout, int *numTypes)
*** 3558,3564 ****
          appendPQExpBuffer(query, "SELECT "
           "(SELECT oid FROM pg_class WHERE relname = 'pg_type') AS tableoid, "
                            "oid, typname, "
!                           "0::oid AS typnamespace, '{=U}' AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
--- 3558,3564 ----
          appendPQExpBuffer(query, "SELECT "
           "(SELECT oid FROM pg_class WHERE relname = 'pg_type') AS tableoid, "
                            "oid, typname, "
!                           "0::oid AS typnamespace, NULL AS typacl, "
                            "(%s typowner) AS rolname, "
                            "typinput::oid AS typinput, "
                            "typoutput::oid AS typoutput, typelem, typrelid, "
*************** getAggregates(Archive *fout, DumpOptions
*** 4249,4255 ****
                    "CASE WHEN aggbasetype = 0 THEN 0 ELSE 1 END AS pronargs, "
                            "aggbasetype AS proargtypes, "
                            "(%s aggowner) AS rolname, "
!                           "'{=X}' AS aggacl "
                            "FROM pg_aggregate "
                            "where oid > '%u'::oid",
                            username_subquery,
--- 4249,4255 ----
                    "CASE WHEN aggbasetype = 0 THEN 0 ELSE 1 END AS pronargs, "
                            "aggbasetype AS proargtypes, "
                            "(%s aggowner) AS rolname, "
!                           "NULL AS aggacl "
                            "FROM pg_aggregate "
                            "where oid > '%u'::oid",
                            username_subquery,
*************** getAggregates(Archive *fout, DumpOptions
*** 4264,4270 ****
                    "CASE WHEN aggbasetype = 0 THEN 0 ELSE 1 END AS pronargs, "
                            "aggbasetype AS proargtypes, "
                            "(%s aggowner) AS rolname, "
!                           "'{=X}' AS aggacl "
                            "FROM pg_aggregate "
                            "where oid > '%u'::oid",
                            username_subquery,
--- 4264,4270 ----
                    "CASE WHEN aggbasetype = 0 THEN 0 ELSE 1 END AS pronargs, "
                            "aggbasetype AS proargtypes, "
                            "(%s aggowner) AS rolname, "
!                           "NULL AS aggacl "
                            "FROM pg_aggregate "
                            "where oid > '%u'::oid",
                            username_subquery,
*************** getFuncs(Archive *fout, DumpOptions *dop
*** 4408,4414 ****
          appendPQExpBuffer(query,
                            "SELECT tableoid, oid, proname, prolang, "
                            "pronargs, proargtypes, prorettype, "
!                           "'{=X}' AS proacl, "
                            "0::oid AS pronamespace, "
                            "(%s proowner) AS rolname "
                            "FROM pg_proc "
--- 4408,4414 ----
          appendPQExpBuffer(query,
                            "SELECT tableoid, oid, proname, prolang, "
                            "pronargs, proargtypes, prorettype, "
!                           "NULL AS proacl, "
                            "0::oid AS pronamespace, "
                            "(%s proowner) AS rolname "
                            "FROM pg_proc "
*************** getFuncs(Archive *fout, DumpOptions *dop
*** 4424,4430 ****
                            " WHERE relname = 'pg_proc') AS tableoid, "
                            "oid, proname, prolang, "
                            "pronargs, proargtypes, prorettype, "
!                           "'{=X}' AS proacl, "
                            "0::oid AS pronamespace, "
                            "(%s proowner) AS rolname "
                            "FROM pg_proc "
--- 4424,4430 ----
                            " WHERE relname = 'pg_proc') AS tableoid, "
                            "oid, proname, prolang, "
                            "pronargs, proargtypes, prorettype, "
!                           "NULL AS proacl, "
                            "0::oid AS pronamespace, "
                            "(%s proowner) AS rolname "
                            "FROM pg_proc "
*************** getProcLangs(Archive *fout, int *numProc
*** 6317,6323 ****
          /* pg_language has a laninline column */
          appendPQExpBuffer(query, "SELECT tableoid, oid, "
                            "lanname, lanpltrusted, lanplcallfoid, "
!                           "laninline, lanvalidator,  lanacl, "
                            "(%s lanowner) AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
--- 6317,6323 ----
          /* pg_language has a laninline column */
          appendPQExpBuffer(query, "SELECT tableoid, oid, "
                            "lanname, lanpltrusted, lanplcallfoid, "
!                           "laninline, lanvalidator, lanacl, "
                            "(%s lanowner) AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
*************** getProcLangs(Archive *fout, int *numProc
*** 6329,6335 ****
          /* pg_language has a lanowner column */
          appendPQExpBuffer(query, "SELECT tableoid, oid, "
                            "lanname, lanpltrusted, lanplcallfoid, "
!                           "lanvalidator,  lanacl, "
                            "(%s lanowner) AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
--- 6329,6335 ----
          /* pg_language has a lanowner column */
          appendPQExpBuffer(query, "SELECT tableoid, oid, "
                            "lanname, lanpltrusted, lanplcallfoid, "
!                           "0 AS laninline, lanvalidator, lanacl, "
                            "(%s lanowner) AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
*************** getProcLangs(Archive *fout, int *numProc
*** 6339,6345 ****
      else if (fout->remoteVersion >= 80100)
      {
          /* Languages are owned by the bootstrap superuser, OID 10 */
!         appendPQExpBuffer(query, "SELECT tableoid, oid, *, "
                            "(%s '10') AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
--- 6339,6347 ----
      else if (fout->remoteVersion >= 80100)
      {
          /* Languages are owned by the bootstrap superuser, OID 10 */
!         appendPQExpBuffer(query, "SELECT tableoid, oid, "
!                           "lanname, lanpltrusted, lanplcallfoid, "
!                           "0 AS laninline, lanvalidator, lanacl, "
                            "(%s '10') AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
*************** getProcLangs(Archive *fout, int *numProc
*** 6349,6375 ****
      else if (fout->remoteVersion >= 70400)
      {
          /* Languages are owned by the bootstrap superuser, sysid 1 */
!         appendPQExpBuffer(query, "SELECT tableoid, oid, *, "
                            "(%s '1') AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
                            "ORDER BY oid",
                            username_subquery);
      }
!     else if (fout->remoteVersion >= 70100)
      {
          /* No clear notion of an owner at all before 7.4 ... */
!         appendPQExpBufferStr(query, "SELECT tableoid, oid, * FROM pg_language "
!                              "WHERE lanispl "
!                              "ORDER BY oid");
      }
      else
      {
!         appendPQExpBufferStr(query, "SELECT "
!                              "(SELECT oid FROM pg_class WHERE relname = 'pg_language') AS tableoid, "
!                              "oid, * FROM pg_language "
!                              "WHERE lanispl "
!                              "ORDER BY oid");
      }

      res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
--- 6351,6397 ----
      else if (fout->remoteVersion >= 70400)
      {
          /* Languages are owned by the bootstrap superuser, sysid 1 */
!         appendPQExpBuffer(query, "SELECT tableoid, oid, "
!                           "lanname, lanpltrusted, lanplcallfoid, "
!                           "0 AS laninline, lanvalidator, lanacl, "
                            "(%s '1') AS lanowner "
                            "FROM pg_language "
                            "WHERE lanispl "
                            "ORDER BY oid",
                            username_subquery);
      }
!     else if (fout->remoteVersion >= 70300)
      {
          /* No clear notion of an owner at all before 7.4 ... */
!         appendPQExpBuffer(query, "SELECT tableoid, oid, "
!                           "lanname, lanpltrusted, lanplcallfoid, "
!                           "0 AS laninline, lanvalidator, lanacl, "
!                           "NULL AS lanowner "
!                           "FROM pg_language "
!                           "WHERE lanispl "
!                           "ORDER BY oid");
!     }
!     else if (fout->remoteVersion >= 70100)
!     {
!         appendPQExpBuffer(query, "SELECT tableoid, oid, "
!                           "lanname, lanpltrusted, lanplcallfoid, "
!                         "0 AS laninline, 0 AS lanvalidator, NULL AS lanacl, "
!                           "NULL AS lanowner "
!                           "FROM pg_language "
!                           "WHERE lanispl "
!                           "ORDER BY oid");
      }
      else
      {
!         appendPQExpBuffer(query, "SELECT "
!                           "(SELECT oid FROM pg_class WHERE relname = 'pg_language') AS tableoid, "
!                           "oid, "
!                           "lanname, lanpltrusted, lanplcallfoid, "
!                         "0 AS laninline, 0 AS lanvalidator, NULL AS lanacl, "
!                           "NULL AS lanowner "
!                           "FROM pg_language "
!                           "WHERE lanispl "
!                           "ORDER BY oid");
      }

      res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
*************** getProcLangs(Archive *fout, int *numProc
*** 6385,6391 ****
      i_lanname = PQfnumber(res, "lanname");
      i_lanpltrusted = PQfnumber(res, "lanpltrusted");
      i_lanplcallfoid = PQfnumber(res, "lanplcallfoid");
-     /* these may fail and return -1: */
      i_laninline = PQfnumber(res, "laninline");
      i_lanvalidator = PQfnumber(res, "lanvalidator");
      i_lanacl = PQfnumber(res, "lanacl");
--- 6407,6412 ----
*************** getProcLangs(Archive *fout, int *numProc
*** 6401,6422 ****
          planginfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_lanname));
          planginfo[i].lanpltrusted = *(PQgetvalue(res, i, i_lanpltrusted)) == 't';
          planginfo[i].lanplcallfoid = atooid(PQgetvalue(res, i, i_lanplcallfoid));
!         if (i_laninline >= 0)
!             planginfo[i].laninline = atooid(PQgetvalue(res, i, i_laninline));
!         else
!             planginfo[i].laninline = InvalidOid;
!         if (i_lanvalidator >= 0)
!             planginfo[i].lanvalidator = atooid(PQgetvalue(res, i, i_lanvalidator));
!         else
!             planginfo[i].lanvalidator = InvalidOid;
!         if (i_lanacl >= 0)
!             planginfo[i].lanacl = pg_strdup(PQgetvalue(res, i, i_lanacl));
!         else
!             planginfo[i].lanacl = pg_strdup("{=U}");
!         if (i_lanowner >= 0)
!             planginfo[i].lanowner = pg_strdup(PQgetvalue(res, i, i_lanowner));
!         else
!             planginfo[i].lanowner = pg_strdup("");

          if (fout->remoteVersion < 70300)
          {
--- 6422,6431 ----
          planginfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_lanname));
          planginfo[i].lanpltrusted = *(PQgetvalue(res, i, i_lanpltrusted)) == 't';
          planginfo[i].lanplcallfoid = atooid(PQgetvalue(res, i, i_lanplcallfoid));
!         planginfo[i].laninline = atooid(PQgetvalue(res, i, i_laninline));
!         planginfo[i].lanvalidator = atooid(PQgetvalue(res, i, i_lanvalidator));
!         planginfo[i].lanacl = pg_strdup(PQgetvalue(res, i, i_lanacl));
!         planginfo[i].lanowner = pg_strdup(PQgetvalue(res, i, i_lanowner));

          if (fout->remoteVersion < 70300)
          {

On 08/10/2015 07:29 PM, Tom Lane wrote:
> I wrote:
>> Peter Eisentraut <peter_e@gmx.net> writes:
>>> This was probably just copied from how proacl and lanacl are handled,
>>> which predate typacl by quite a bit.  Maybe there was a reason in those
>>> days.
>> Hm ... I wonder whether those are well-thought-out either.
> They're not.  Testing with ancient servers shows that we dump very silly
> grant/revoke state for functions and languages as well, if the source
> server is too old to have proacl or lanacl (ie, pre-7.3).  As with typacl,
> the silliness is accidentally masked as long as the owner doesn't do
> something like revoke the privileges granted to PUBLIC.
>
> Things work far more sanely with the attached patch, to wit we just leave
> all object privileges as default if dumping from a version too old to have
> privileges on that type of object.  I think we should back-patch this into
> all supported branches; it's considerably more likely that older branches
> would be used to dump from ancient servers.
>
>             


FYI this has fixed the problem that was encountered in cross-version 
upgrade testing.

cheers

andrew
>