Horton

Thread

Commits

  1. pg_dump: fix mis-dumping of non-global default privileges.

  1. Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Boris P. Korzun <drtr0jan@yandex.ru> — 2021-03-24T20:38:50Z

      Hi,


  
     
  
  
    
      PostgreSQL version: 13.2
    
    
      Operating system:  FreeBSD 13-STABLE
    
    
       
    
  
  
    I've founded that pg_dump (and pg_dumpall) incorrectlty exports subsequent default privileges.
  
  
    Ex.:
  
  
    > ALTER DEFAULT PRIVILEGES FOR ROLE postgres REVOKE ALL ON FUNCTIONS FROM PUBLIC;
  
  
    > ALTER DEFAULT PRIVILEGES FOR ROLE postgres IN SCHEMA bar GRANT EXECUTE ON FUNCTIONS TO PUBLIC;
  
  
    > bug=# \ddp
  
  
    >              Default access privileges
  
  
    >   Owner   | Schema |   Type   |  Access privileges
  
  
    > ----------+--------+----------+---------------------
  
  
    >  postgres | bar    | function | =X/postgres
  
  
    >  postgres |        | function | postgres=X/postgres
  
  
    > -- I can execute newly created functions on scheme 'bar' after that.
  
  
     
  
  
    will be exported as:
  
  
    > ALTER DEFAULT PRIVILEGES FOR ROLE postgres IN SCHEMA bar REVOKE ALL ON FUNCTIONS  FROM postgres;
  
  
    > ALTER DEFAULT PRIVILEGES FOR ROLE postgres REVOKE ALL ON FUNCTIONS  FROM PUBLIC;
  
  
    > bug=# \ddp
  
  
    >              Default access privileges
  
  
    >   Owner   | Schema |   Type   |  Access privileges  
  
  
    > ----------+--------+----------+---------------------
  
  
    >  postgres |        | function | postgres=X/postgres
  
  
    > -- I can NOT execute newly created functions on scheme 'bar' after that.
  
  
     
  
  
    After restoring the dump I cannot execute newly created functions on scheme 'bar'.
  
  
    I've attached the shell-script that create the environment.
  
  
     
  
  
    I create a function after dump or restore procedures (ex. backup purposes):
  
  
    
      > CREATE FUNCTION bar.new() RETURNS bool
    
    
      >     IMMUTABLE
    
    
      >     LANGUAGE sql
    
    
      > AS 'SELECT TRUE;';
    
    
      The function bar.new() cannot be executed by 'bug' user if created after the restore procedure.
    
    
      I've got an error: ERROR:  permission denied for function new.
    
    
       
    
  
  
    WBR,
  
  
    Boris

  2. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Neil Chen <carpenter.nail.cz@gmail.com> — 2021-03-31T03:02:00Z

    Greetings,

I did some research on this bug and found that the reason for the problem
is that the pg_dump misjudged the non-global default access privileges when
exporting. The details are as follows:

> The default for a global entry is the hard-wired default ACL for the
> particular object type.  The default for non-global entries is an empty
> ACL.  This must be so because global entries replace the hard-wired
> defaults, while others are added on.
>
We can find this description in code
comments(src/backend/catalog/aclchk.c:1162). For example, if we log as user
postgres, for global entire our default ACL is
"{=X/postgres,postgres=X/postgres}", for non-global entire it's "NULL".

Now let's look at a part of the SQL statement used when pg_dump exports the
default ACL(it can be found in src/bin/pg_dump/dumputils.c:762):

> (SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM
> (SELECT acl, row_n FROM
> pg_catalog.unnest(coalesce(defaclacl,pg_catalog.acldefault(CASE WHEN
> defaclobjtype = 'S' THEN 's' ELSE defaclobjtype END::"char",defaclrole)))
> WITH ORDINALITY AS perm(acl,row_n)
> WHERE NOT EXISTS (
> SELECT 1 FROM
> pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(CASE WHEN
> defaclobjtype = 'S' THEN 's' ELSE defaclobjtype END::"char",defaclrole)))
> AS init(init_acl) WHERE acl = init_acl)) as foo)

It can be seen that when comparing the changes of default ACL, it does not
distinguish between global and non-global default ACL. It uses
{=X/postgres,postgres=X/postgres} as the non-global default ACL by mistake,
resulting in the export error.

Combined with the above research, I gave this patch to fix the bug. Hackers
can help to see if this modification is correct. I'm studying how to write
test scripts for it...

Thanks.

-- 
There is no royal road to learning.
HighGo Software Co.

  3. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Neil Chen <carpenter.nail.cz@gmail.com> — 2021-03-31T03:30:54Z

    Sorry I used the wrong way to send the email. The email about the bug is
here:
https://www.postgresql.org/message-id/111621616618184%40mail.yandex.ru

On Wed, Mar 31, 2021 at 11:02 AM Neil Chen <carpenter.nail.cz@gmail.com>
wrote:

> Greetings,
>
> I did some research on this bug and found that the reason for the problem
> is that the pg_dump misjudged the non-global default access privileges when
> exporting. The details are as follows:
>
>> The default for a global entry is the hard-wired default ACL for the
>> particular object type.  The default for non-global entries is an empty
>> ACL.  This must be so because global entries replace the hard-wired
>> defaults, while others are added on.
>>
> We can find this description in code
> comments(src/backend/catalog/aclchk.c:1162). For example, if we log as user
> postgres, for global entire our default ACL is
> "{=X/postgres,postgres=X/postgres}", for non-global entire it's "NULL".
>
> Now let's look at a part of the SQL statement used when pg_dump exports
> the default ACL(it can be found in src/bin/pg_dump/dumputils.c:762):
>
>> (SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM
>> (SELECT acl, row_n FROM
>> pg_catalog.unnest(coalesce(defaclacl,pg_catalog.acldefault(CASE WHEN
>> defaclobjtype = 'S' THEN 's' ELSE defaclobjtype END::"char",defaclrole)))
>> WITH ORDINALITY AS perm(acl,row_n)
>> WHERE NOT EXISTS (
>> SELECT 1 FROM
>> pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(CASE WHEN
>> defaclobjtype = 'S' THEN 's' ELSE defaclobjtype END::"char",defaclrole)))
>> AS init(init_acl) WHERE acl = init_acl)) as foo)
>
> It can be seen that when comparing the changes of default ACL, it does not
> distinguish between global and non-global default ACL. It uses
> {=X/postgres,postgres=X/postgres} as the non-global default ACL by mistake,
> resulting in the export error.
>
> Combined with the above research, I gave this patch to fix the
> bug. Hackers can help to see if this modification is correct. I'm studying
> how to write test scripts for it...
>
> Thanks.
>
> --
> There is no royal road to learning.
> HighGo Software Co.
>


-- 
There is no royal road to learning.
HighGo Software Co.

  4. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Boris P. Korzun <drtr0jan@yandex.ru> — 2021-04-02T14:09:14Z

    Hi Neil,

> Combined with the above research, I gave this patch to fix the 
> bug. Hackers can help to see if this modification is correct. I'm 
> studying how to write test scripts for it...

it works. Thx.

WBR,
Boris

  5. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Boris P. Korzun <drtr0jan@yandex.ru> — 2021-09-21T05:04:58Z

    <div>Hi Neil,</div><div> </div><div>what about the commit to the upstream?</div><div> </div><div>31.03.2021, 06:02, "Neil Chen" &lt;carpenter.nail.cz@gmail.com&gt;:</div><blockquote><div>Greetings,<div> </div><div>I did some research on this bug and found that the reason for the problem is that the pg_dump misjudged the non-global default access privileges when exporting. The details are as follows:</div><blockquote>The default for a global entry is the hard-wired default ACL for the<br />particular object type.  The default for non-global entries is an empty<br />ACL.  This must be so because global entries replace the hard-wired<br />defaults, while others are added on.</blockquote><div>We can find this description in code comments(src/backend/catalog/aclchk.c:1162). For example, if we log as user postgres, for global entire our default ACL is "{=X/postgres,postgres=X/postgres}", for non-global entire it's "NULL".</div><div> </div><div>Now let's look at a part of the SQL statement used when pg_dump exports the default ACL(it can be found in src/bin/pg_dump/dumputils.c:762):</div><blockquote>(SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM<br />(SELECT acl, row_n FROM<br />pg_catalog.unnest(coalesce(defaclacl,pg_catalog.acldefault(CASE WHEN defaclobjtype = 'S' THEN 's' ELSE defaclobjtype END::"char",defaclrole)))<br />WITH ORDINALITY AS perm(acl,row_n)<br />WHERE NOT EXISTS (<br />SELECT 1 FROM<br />pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault(CASE WHEN defaclobjtype = 'S' THEN 's' ELSE defaclobjtype END::"char",defaclrole)))<br />AS init(init_acl) WHERE acl = init_acl)) as foo) </blockquote><div>It can be seen that when comparing the changes of default ACL, it does not distinguish between global and non-global default ACL. It uses {=X/postgres,postgres=X/postgres} as the non-global default ACL by mistake, resulting in the export error.</div><div> </div><div>Combined with the above research, I gave this patch to fix the bug. Hackers can help to see if this modification is correct. I'm studying how to write test scripts for it...</div><div> </div><div>Thanks.</div><div> </div>--<div><div>There is no royal road to learning.<div>HighGo Software Co.</div></div></div></div></blockquote><div>---</div><div>WBR</div><div>Boris</div>

  6. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Neil Chen <carpenter.nail.cz@gmail.com> — 2021-09-22T01:30:50Z

    Hi Boris,

Actually, because I am a PG beginner, I am not familiar with the rules of
the community. What extra work do I need to do to submit to the upstream?
This bug discussion doesn't seem to see the concern of others.

On Tue, Sep 21, 2021 at 1:05 PM Boris P. Korzun <drtr0jan@yandex.ru> wrote:

> Hi Neil,
>
> what about the commit to the upstream?
>
> ---
> WBR
> Boris
>


-- 
There is no royal road to learning.
HighGo Software Co.

  7. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-01T14:13:45Z

    On Wed, Sep 22, 2021 at 10:31 AM Neil Chen <carpenter.nail.cz@gmail.com> wrote:
>
> Hi Boris,
>
> Actually, because I am a PG beginner, I am not familiar with the rules of the community. What extra work do I need to do to submit to the upstream? This bug discussion doesn't seem to see the concern of others.

As far as I checked this bug still exists in all supported branches
(from 10 to 14, and HEAD). I'd recommend adding this patch to the next
commit fest so as not to forget, if not yet.

I agree with your analysis on this bug. For non-default
(defaclnamespace != 0) entries, their acl should be compared to NULL.

The fix also looks good to me. But I think it'd be better to add tests for this.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  8. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Boris P. Korzun <drtr0jan@yandex.ru> — 2021-10-13T08:22:49Z

    Hi Neil,

you should send the patch via e-mail to the pgsql-hackers ( 
http://www.postgresql.org/list/pgsql-hackers/ ) mailing list for adding 
to the next commit fest as Masahiko Sawada said.

I can help you if you have any questions.

On 22/09/2021 04:30, Neil Chen wrote:
> Hi Boris,
>
> Actually, because I am a PG beginner, I am not familiar with the rules 
> of the community. What extra work do I need to do to submit to the 
> upstream? This bug discussion doesn't seem to see the concern of others.
>
> On Tue, Sep 21, 2021 at 1:05 PM Boris P. Korzun <drtr0jan@yandex.ru> 
> wrote:
>
>     Hi Neil,
>     what about the commit to the upstream?
>     ---
>     WBR
>     Boris
>
>
>
> -- 
> There is no royal road to learning.
> HighGo Software Co.

---

WBR

Boris

  9. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-14T00:36:55Z

    Hi,

On Fri, Oct 1, 2021 at 11:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Sep 22, 2021 at 10:31 AM Neil Chen <carpenter.nail.cz@gmail.com> wrote:
> >
> > Hi Boris,
> >
> > Actually, because I am a PG beginner, I am not familiar with the rules of the community. What extra work do I need to do to submit to the upstream? This bug discussion doesn't seem to see the concern of others.
>
> As far as I checked this bug still exists in all supported branches
> (from 10 to 14, and HEAD). I'd recommend adding this patch to the next
> commit fest so as not to forget, if not yet.
>
> I agree with your analysis on this bug. For non-default
> (defaclnamespace != 0) entries, their acl should be compared to NULL.
>
> The fix also looks good to me. But I think it'd be better to add tests for this.

Since the patch conflicts with the current HEAD, I've rebased and
slightly updated the patch, adding the regression tests.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  10. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-14T00:59:23Z

    On Fri, Oct 1, 2021 at 11:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Sep 22, 2021 at 10:31 AM Neil Chen <carpenter.nail.cz@gmail.com> wrote:
> >
> > Hi Boris,
> >
> > Actually, because I am a PG beginner, I am not familiar with the rules of the community. What extra work do I need to do to submit to the upstream? This bug discussion doesn't seem to see the concern of others.
>
> As far as I checked this bug still exists in all supported branches
> (from 10 to 14, and HEAD). I'd recommend adding this patch to the next
> commit fest so as not to forget, if not yet.
>
> I agree with your analysis on this bug. For non-default
> (defaclnamespace != 0) entries, their acl should be compared to NULL.
>
> The fix also looks good to me. But I think it'd be better to add tests for this.
>

Since the patch conflicts with the current HEAD, I've rebased and
slightly updated the patch, adding the regression tests.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  11. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-14T02:55:51Z

    Masahiko Sawada <sawada.mshk@gmail.com> writes:
>> I agree with your analysis on this bug. For non-default
>> (defaclnamespace != 0) entries, their acl should be compared to NULL.
>> 
>> The fix also looks good to me. But I think it'd be better to add tests for this.

> Since the patch conflicts with the current HEAD, I've rebased and
> slightly updated the patch, adding the regression tests.

Hmmm ... if we're adding a comment about the defaclnamespace check,
seems like it would also be a nice idea to explain the S-to-s
transformation, because the reason for that is surely not apparent.

			regards, tom lane

  12. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-14T05:22:21Z

    On Thu, Oct 14, 2021 at 11:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> >> I agree with your analysis on this bug. For non-default
> >> (defaclnamespace != 0) entries, their acl should be compared to NULL.
> >>
> >> The fix also looks good to me. But I think it'd be better to add tests for this.
>
> > Since the patch conflicts with the current HEAD, I've rebased and
> > slightly updated the patch, adding the regression tests.
>
> Hmmm ... if we're adding a comment about the defaclnamespace check,
> seems like it would also be a nice idea to explain the S-to-s
> transformation, because the reason for that is surely not apparent.
>

Agreed. Please find an attached new patch.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  13. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Michael Paquier <michael@paquier.xyz> — 2021-10-14T07:53:24Z

    On Thu, Oct 14, 2021 at 02:22:21PM +0900, Masahiko Sawada wrote:
> Agreed. Please find an attached new patch.

I have not dived into the details of the patch yet, but I can see the
following diffs in some of the dumps dropped by the new test added
between HEAD and the patch:
1) For DEFAULT PRIVILEGES FOR FUNCTIONS:
-ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
     dump_test REVOKE ALL ON FUNCTIONS  FROM PUBLIC;
+ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
     dump_test GRANT ALL ON FUNCTIONS  TO regress_dump_test_role;
2) For DEFAULT PRIVILEGES FOR TABLES:
-ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
     dump_test REVOKE ALL ON TABLES  FROM regress_dump_test_role;
 ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
     dump_test GRANT SELECT ON TABLES  TO regress_dump_test_role;

So the patch removes a REVOKE ALL ON TABLES on
regress_dump_test_role after the addition of only the GRANT EXECUTE ON
FUNCTIONS.  That seems off.  Am I missing something?
--
Michael

  14. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Nathan Bossart <bossartn@amazon.com> — 2021-10-14T16:02:15Z

    On 10/14/21, 12:55 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> So the patch removes a REVOKE ALL ON TABLES on
> regress_dump_test_role after the addition of only the GRANT EXECUTE ON
> FUNCTIONS.  That seems off.  Am I missing something?

This issue is also tracked here:

        https://commitfest.postgresql.org/35/3288/

I had attempted to fix this by replacing acldefault() with NULL when
defaclnamespace was 0.  From a quick glance, the patch in this thread
seems to be adjusting obj_kind based on whether defaclnamespace is 0.
I think this has the same effect because acldefault() is STRICT.

Nathan

  15. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Nathan Bossart <bossartn@amazon.com> — 2021-10-14T16:13:52Z

    On 10/14/21, 12:55 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> 1) For DEFAULT PRIVILEGES FOR FUNCTIONS:
> -ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test REVOKE ALL ON FUNCTIONS  FROM PUBLIC;
> +ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test GRANT ALL ON FUNCTIONS  TO regress_dump_test_role;

This one looks correct to me.

> 2) For DEFAULT PRIVILEGES FOR TABLES:
> -ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test REVOKE ALL ON TABLES  FROM regress_dump_test_role;
>  ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test GRANT SELECT ON TABLES  TO regress_dump_test_role;
>
> So the patch removes a REVOKE ALL ON TABLES on
> regress_dump_test_role after the addition of only the GRANT EXECUTE ON
> FUNCTIONS.  That seems off.  Am I missing something?

I might be missing something as well, but this one looks correct to
me, too.  I suspect that REVOKE statement was generated by comparing
against the wrong default ACL and that it actually has no effect.

Nathan

  16. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-15T00:05:07Z

    On Thu, Oct 14, 2021 at 4:53 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Oct 14, 2021 at 02:22:21PM +0900, Masahiko Sawada wrote:
> > Agreed. Please find an attached new patch.
>
> I have not dived into the details of the patch yet, but I can see the
> following diffs in some of the dumps dropped by the new test added
> between HEAD and the patch:

I've checked where these differences come from:

> 1) For DEFAULT PRIVILEGES FOR FUNCTIONS:
> -ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test REVOKE ALL ON FUNCTIONS  FROM PUBLIC;
> +ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test GRANT ALL ON FUNCTIONS  TO regress_dump_test_role;

The test query and the default privileges I got are:

ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
dump_test GRANT EXECUTE ON FUNCTIONS TO regress_dump_test_role;

                                    Default access privileges
         Owner          |  Schema   |   Type   |                Access
privileges
------------------------+-----------+----------+-------------------------------------------------
 regress_dump_test_role | dump_test | function |
regress_dump_test_role=X/regress_dump_test_role
(1 row)

The query dumped by the current pg_dump (i.g., HEAD, w/o patch) is:

ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
dump_test REVOKE ALL ON FUNCTIONS  FROM PUBLIC;

The query dumped by pg_dump with the patch is:

ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
dump_test GRANT ALL ON FUNCTIONS  TO regress_dump_test_role;

The query dumped by the current pg_dump is wrong and the patch fixes
it. This difference looks good to me.

> 2) For DEFAULT PRIVILEGES FOR TABLES:
> -ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test REVOKE ALL ON TABLES  FROM regress_dump_test_role;
>  ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
>      dump_test GRANT SELECT ON TABLES  TO regress_dump_test_role;

The test query and the default privileges I got are:

ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
dump_test GRANT SELECT ON TABLES TO regress_dump_test_role;

                                  Default access privileges
         Owner          |  Schema   | Type  |                Access privileges
------------------------+-----------+-------+-------------------------------------------------
 regress_dump_test_role | dump_test | table |
regress_dump_test_role=r/regress_dump_test_role
(1 row)

The query dumped by the current pg_dump (i.g., HEAD, w/o patch) is:

ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
dump_test REVOKE ALL ON TABLES  FROM regress_dump_test_role;
ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
dump_test GRANT SELECT ON TABLES  TO regress_dump_test_role;

The query dumped by pg_dump with the patch is:

ALTER DEFAULT PRIVILEGES FOR ROLE regress_dump_test_role IN SCHEMA
dump_test GRANT SELECT ON TABLES  TO regress_dump_test_role;


The current pg_dump produced a REVOKE ALL ON TABLES FROM
regress_dump_test_role but it seems unnecessary. The patch removes it
so looks good to me too.

Regards,
--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  17. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Nathan Bossart <bossartn@amazon.com> — 2021-10-18T23:46:56Z

    On 10/14/21, 5:06 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:
> The current pg_dump produced a REVOKE ALL ON TABLES FROM
> regress_dump_test_role but it seems unnecessary. The patch removes it
> so looks good to me too.

+1

If we are going to proceed with the patch in this thread, I think we
should also mention in the comment that we are depending on
acldefault() being STRICT.  This patch is quite a bit smaller than
what I had proposed, but AFAICT it produces the same result.

Nathan

  18. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-19T02:04:32Z

    On Tue, Oct 19, 2021 at 8:47 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 10/14/21, 5:06 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:
> > The current pg_dump produced a REVOKE ALL ON TABLES FROM
> > regress_dump_test_role but it seems unnecessary. The patch removes it
> > so looks good to me too.
>
> +1
>

I've looked at the patch proposed you proposed. If we can depend on
acldefault() being STRICT (which is legitimate to me), I think we
don't need to build an expression depending on the caller (i.g.,
is_default_acl). If acldefault() were to become not STRICT, we could
detect it by regression tests. What do you think?

> If we are going to proceed with the patch in this thread, I think we
> should also mention in the comment that we are depending on
> acldefault() being STRICT.

I've updated the patch.

>  This patch is quite a bit smaller than
> what I had proposed, but AFAICT it produces the same result.

Yes. I've also confirmed both produce the same result.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  19. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-19T03:19:12Z

    Masahiko Sawada <sawada.mshk@gmail.com> writes:
> I've looked at the patch proposed you proposed. If we can depend on
> acldefault() being STRICT (which is legitimate to me), I think we
> don't need to build an expression depending on the caller (i.g.,
> is_default_acl). If acldefault() were to become not STRICT, we could
> detect it by regression tests. What do you think?

FWIW, I'm working on a refactoring of this logic that will bring the
acldefault() call into the getDefaultACLs code, which would mean that
we won't need that assumption anymore anyway.  The code as I have it
produces SQL like

     acldefault(CASE WHEN defaclobjtype = 'S'
                THEN 's'::"char" ELSE defaclobjtype END, defaclrole) AS acldefault

and we could wrap the test-for-zero around that:

     CASE WHEN defaclnamespace = 0 THEN
       acldefault(CASE WHEN defaclobjtype = 'S'
                  THEN 's'::"char" ELSE defaclobjtype END, defaclrole)
     ELSE NULL END AS acldefault

(although I think it might be better to write ELSE '{}' not ELSE NULL).

So I think we don't need to worry about whether acldefault() will stay
strict.  This patch will only need to work in the back branches.

			regards, tom lane

  20. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Nathan Bossart <bossartn@amazon.com> — 2021-10-19T03:54:37Z

    On 10/18/21, 8:20 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
>> I've looked at the patch proposed you proposed. If we can depend on
>> acldefault() being STRICT (which is legitimate to me), I think we
>> don't need to build an expression depending on the caller (i.g.,
>> is_default_acl). If acldefault() were to become not STRICT, we could
>> detect it by regression tests. What do you think?
>
> FWIW, I'm working on a refactoring of this logic that will bring the
> acldefault() call into the getDefaultACLs code, which would mean that
> we won't need that assumption anymore anyway.  The code as I have it
> produces SQL like

Nice!

> So I think we don't need to worry about whether acldefault() will stay
> strict.  This patch will only need to work in the back branches.

This seems fine to me, too.  I don't think relying on STRICT is any
better or worse than adding a flag for this one special case.

+		/*
+		 * Since the default for a global entry is the hard-wired default
+		 * ACL for the particular object type, we pass defaclobjtype except
+		 * for the case of 'S' (DEFACLOBJ_SEQUENCE) where we need to
+		 * transform it to 's' since acldefault() SQL-callable function
+		 * handles 's' as a sequence.  On the other hand, since the default
+		 * for non-global entries is an empty ACL we pass NULL.  This works
+		 * because acldefault() is STRICT.
+		 */

I'd split out the two special cases in the comment.  What do you think
about something like the following?

        /*
         * Build the expression for determining the object type.
         *
         * While pg_default_acl uses 'S' for sequences, acldefault()
         * uses 's', so we must transform 'S' to 's'.
         *
         * The default for a schema-local default ACL (i.e., entries
         * in pg_default_acl with defaclnamespace != 0) is an empty
         * ACL.  We use NULL as the object type for those entries,
         * which forces acldefault() to also return NULL because it is
         * STRICT.
         */

+		create_sql   => 'ALTER DEFAULT PRIVILEGES
+					   FOR ROLE regress_dump_test_role IN SCHEMA dump_test
+					   GRANT EXECUTE ON FUNCTIONS TO regress_dump_test_role;',
+		regexp => qr/^
+			\QALTER DEFAULT PRIVILEGES \E
+			\QFOR ROLE regress_dump_test_role IN SCHEMA dump_test \E
+			\QGRANT ALL ON FUNCTIONS  TO regress_dump_test_role;\E
+			/xm,

It could be a bit confusing that create_sql uses "GRANT EXECUTE" but
we expect to see "GRANT ALL."  IIUC this is correct, but maybe we
should use "GRANT ALL" in create_sql so that they match.

Nathan

  21. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-19T05:51:36Z

    On Tue, Oct 19, 2021 at 12:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > I've looked at the patch proposed you proposed. If we can depend on
> > acldefault() being STRICT (which is legitimate to me), I think we
> > don't need to build an expression depending on the caller (i.g.,
> > is_default_acl). If acldefault() were to become not STRICT, we could
> > detect it by regression tests. What do you think?
>
> FWIW, I'm working on a refactoring of this logic that will bring the
> acldefault() call into the getDefaultACLs code, which would mean that
> we won't need that assumption anymore anyway.  The code as I have it
> produces SQL like
>
>      acldefault(CASE WHEN defaclobjtype = 'S'
>                 THEN 's'::"char" ELSE defaclobjtype END, defaclrole) AS acldefault
>
> and we could wrap the test-for-zero around that:
>
>      CASE WHEN defaclnamespace = 0 THEN
>        acldefault(CASE WHEN defaclobjtype = 'S'
>                   THEN 's'::"char" ELSE defaclobjtype END, defaclrole)
>      ELSE NULL END AS acldefault
>
> (although I think it might be better to write ELSE '{}' not ELSE NULL).
>
> So I think we don't need to worry about whether acldefault() will stay
> strict.  This patch will only need to work in the back branches.

+1

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  22. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-19T19:53:54Z

    ... BTW, I think this patch is not correct yet.  What I read in
catalogs.sgml is

   ... If a global entry is present then
   it <emphasis>overrides</emphasis> the normal hard-wired default privileges
   for the object type.  A per-schema entry, if present, represents privileges
   to be <emphasis>added to</emphasis> the global or hard-wired default privileges.

I didn't check the code, but if that last bit is correct, then non-global
entries aren't necessarily relative to the acldefault privileges either.

I kind of wonder now whether the existing behavior is correct for either
case.  Why aren't we simply regurgitating the pg_default_acl entries
verbatim?  That is, I think maybe we don't need the acldefault call at
all; we should just use null/empty as the starting ACL in all cases
when printing pg_default_acl entries.  Like this:

        buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
                        initracl_subquery, "defaclacl", "defaclrole",
                        "pip.initprivs",
-                       "CASE WHEN defaclobjtype = 'S' THEN 's' ELSE defaclobjtype END::\"char\"",
+                       "NULL",
                        dopt->binary_upgrade);

I didn't test that.  I suspect it will cause some regression test
changes, but will they be wrong?

			regards, tom lane

  23. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Nathan Bossart <bossartn@amazon.com> — 2021-10-19T21:01:13Z

    On 10/19/21, 12:54 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> I kind of wonder now whether the existing behavior is correct for either
> case.  Why aren't we simply regurgitating the pg_default_acl entries
> verbatim?  That is, I think maybe we don't need the acldefault call at
> all; we should just use null/empty as the starting ACL in all cases
> when printing pg_default_acl entries.  Like this:

Hm.  If we do this, then this command:

        ALTER DEFAULT PRIVILEGES FOR ROLE myrole REVOKE ALL ON FUNCTIONS FROM PUBLIC;

is dumped as:

        ALTER DEFAULT PRIVILEGES FOR ROLE myrole GRANT ALL ON FUNCTIONS  TO myrole;

This command is effectively ignored when you apply it, as no entry is
added to pg_default_acl.  I haven't looked too closely into what's
happening yet, but it does look like there is more to the story.

Nathan

  24. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-19T21:58:34Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/19/21, 12:54 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> I kind of wonder now whether the existing behavior is correct for either
>> case.

> Hm.  If we do this, then this command:
>         ALTER DEFAULT PRIVILEGES FOR ROLE myrole REVOKE ALL ON FUNCTIONS FROM PUBLIC;
> is dumped as:
>         ALTER DEFAULT PRIVILEGES FOR ROLE myrole GRANT ALL ON FUNCTIONS  TO myrole;

[ pokes at it some more... ]  Yeah, I just didn't have my head screwed
on straight.  We need the global entries to be dumped as deltas from
the proper object-type-specific ACL, while the non-global ones should be
dumped as grants only, which can be modeled as a delta from an empty
ACL.  So the patch should be good as given (though maybe the comment
needs more work to clarify this).  Sorry for the noise.

			regards, tom lane

  25. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-22T19:26:49Z

    I wrote:
> [ pokes at it some more... ]  Yeah, I just didn't have my head screwed
> on straight.  We need the global entries to be dumped as deltas from
> the proper object-type-specific ACL, while the non-global ones should be
> dumped as grants only, which can be modeled as a delta from an empty
> ACL.  So the patch should be good as given (though maybe the comment
> needs more work to clarify this).  Sorry for the noise.

This was blocking some other work I'm doing on pg_dump, so I rewrote
the comment some more and pushed it.

			regards, tom lane

  26. Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

    Nathan Bossart <bossartn@amazon.com> — 2021-10-22T20:08:01Z

    On 10/22/21, 12:27 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> This was blocking some other work I'm doing on pg_dump, so I rewrote
> the comment some more and pushed it.

Thanks!

Nathan