Re: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA
"Bossart, Nathan" 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
Re: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA
On 9/5/21, 10:08 AM, "Tom Lane" wrote: > "Bossart, Nathan" 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
Re: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA
"Bossart, Nathan" writes:
> On 9/5/21, 10:08 AM, "Tom Lane" 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
Re: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA
On 9/5/21, 12:14 PM, "Tom Lane" wrote: > "Bossart, Nathan" 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
Re: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA
On 9/5/21, 11:33 PM, "Bossart, Nathan" 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
Re: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA
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
