Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

Sorry I used the wrong way to send the email. The email about the bug is
here:
/messages/by-id/111621616618184@mail.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.

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

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

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.

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/

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

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/

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/