Thread: Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

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

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
doto 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/

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

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
todo 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/

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
todo 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/

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

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

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/

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

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

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

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/

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

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/

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

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

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/

... 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

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

"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

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

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