Обсуждение: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA
Hi hackers, I received a report of missing privileges after an upgrade, and I believe I've traced it back to pg_dump's handling of ALTER DEFAULT PRIVILEGES IN SCHEMA. I did find a recent report [0] that looks related, but I didn't see any follow-ups on that thread. It looks like the issue dates back to the introduction of pg_init_privs in v9.6 [1] [2]. A simple reproduction of the issue is to run pg_dump after the following command is run: ALTER DEFAULT PRIVILEGES FOR ROLE nathan IN SCHEMA test GRANT EXECUTE ON FUNCTIONS TO PUBLIC; pg_dump will emit this command for this ACL: ALTER DEFAULT PRIVILEGES FOR ROLE nathan IN SCHEMA test REVOKE ALL ON FUNCTIONS FROM nathan; The problem appears to be that pg_dump is comparing the entries in pg_default_acl to the default ACL (i.e., acldefault()). This is fine for "global" entries (i.e., entries with no schema specified), but it doesn't work for "non-global" entries (i.e., entries with a schema specified). This is because the default for a non-global entry is actually an empty ACL. aclchk.c has the following comment: /* * 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. */ I've attached a quick hack that seems to fix this by adjusting the pg_dump query to use NULL instead of acldefault() for non-global entries. I'm posting this early in order to gather thoughts on the approach and to make sure I'm not missing something obvious. Nathan [0] https://postgr.es/m/111621616618184%40mail.yandex.ru [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=23f34fa [2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e2090d9
Вложения
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested The patch looks fine and successfully fixes the issue of missing privileges issue. The new status of this patch is: Ready for Committer
"Bossart, Nathan" <bossartn@amazon.com> writes: > The problem appears to be that pg_dump is comparing the entries in > pg_default_acl to the default ACL (i.e., acldefault()). This is fine > for "global" entries (i.e., entries with no schema specified), but it > doesn't work for "non-global" entries (i.e., entries with a schema > specified). This is because the default for a non-global entry is > actually an empty ACL. Good point. > I've attached a quick hack that seems to fix this by adjusting the > pg_dump query to use NULL instead of acldefault() for non-global > entries. I'm posting this early in order to gather thoughts on the > approach and to make sure I'm not missing something obvious. I find this impossible to comment on as to correctness, because the patch is nigh unreadable. "case_stmt" is a pretty darn opaque variable name, and the lack of comments doesn't help, and I don't really think that you chose good semantics for it anyway. Probably it would be better for the new argument to be along the lines of "bool is_default_acl", and allow buildACLQueries to know what it should put in when that's true. I'm kind of allergic to this SQL coding style, too. It expects the backend to expend many thousands of cycles parsing and then optimizing away a useless CASE, to save a couple of lines of code and a few cycles on the client side. Nor is doing the query this way even particularly readable on the client side. Lastly, there probably should be a test case or two. regards, tom lane
On 9/5/21, 10:08 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: > "Bossart, Nathan" <bossartn@amazon.com> writes: >> I've attached a quick hack that seems to fix this by adjusting the >> pg_dump query to use NULL instead of acldefault() for non-global >> entries. I'm posting this early in order to gather thoughts on the >> approach and to make sure I'm not missing something obvious. > > I find this impossible to comment on as to correctness, because the patch > is nigh unreadable. "case_stmt" is a pretty darn opaque variable name, > and the lack of comments doesn't help, and I don't really think that you > chose good semantics for it anyway. Probably it would be better for the > new argument to be along the lines of "bool is_default_acl", and allow > buildACLQueries to know what it should put in when that's true. My apologies. This definitely shouldn't have been marked as ready- for-committer. FWIW this is exactly the sort of feedback I was hoping to get before I expended more effort on this patch. > I'm kind of allergic to this SQL coding style, too. It expects the > backend to expend many thousands of cycles parsing and then optimizing > away a useless CASE, to save a couple of lines of code and a few cycles > on the client side. Nor is doing the query this way even particularly > readable on the client side. Is there a specific style you have in mind for this change, or is your point that the CASE statement should only be reserved for when is_default_acl is true? > Lastly, there probably should be a test case or two. Of course. That will be in the next revision. Nathan
"Bossart, Nathan" <bossartn@amazon.com> writes: > On 9/5/21, 10:08 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >> I'm kind of allergic to this SQL coding style, too. It expects the >> backend to expend many thousands of cycles parsing and then optimizing >> away a useless CASE, to save a couple of lines of code and a few cycles >> on the client side. Nor is doing the query this way even particularly >> readable on the client side. > Is there a specific style you have in mind for this change, or is your > point that the CASE statement should only be reserved for when > is_default_acl is true? I'd be inclined to split this logic out into a separate if-statement, along the lines of ... build some of the query ... if (is_default_acl) { /* The reference ACL is empty for schema-local default ACLs */ appendPQExpBuffer(..., "CASE WHEN ... THEN pg_catalog.acldefault(%s,%s) ELSE NULL END", ...); } else { /* For other cases, the reference is always acldefault() */ appendPQExpBuffer(..., "pg_catalog.acldefault(%s,%s)", ...); } ... build rest of the query ... I think this is more readable than one giant printf, and not incidentally it allows room for some helpful comments. (I kind of wonder if we shouldn't try to refactor buildACLQueries to reduce code duplication and add comments while we're at it. The existing code seems pretty awful from a readability standpoint already. I don't have any clear idea about what to do instead, though.) regards, tom lane
On 9/5/21, 12:14 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: > "Bossart, Nathan" <bossartn@amazon.com> writes: >> Is there a specific style you have in mind for this change, or is your >> point that the CASE statement should only be reserved for when >> is_default_acl is true? > > I'd be inclined to split this logic out into a separate if-statement, > along the lines of Got it, thanks. Nathan
On 9/5/21, 12:58 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote: > On 9/5/21, 12:14 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >> I'd be inclined to split this logic out into a separate if-statement, >> along the lines of > > Got it, thanks. Attached is an attempt at cleaning the patch up and adding an informative comment and a test case. Nathan
Вложения
On 9/5/21, 11:33 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote: > Attached is an attempt at cleaning the patch up and adding an > informative comment and a test case. For future reference, there was another thread for this bug [0], and a fix was committed [1]. Nathan [0] https://postgr.es/m/CAA3qoJnr2%2B1dVJObNtfec%3DqW4Z0nz%3DA9%2Br5bZKoTSy5RDjskMw%40mail.gmail.com [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2acc84c