Thread: 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
Attachment
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
Attachment
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