On 9/5/21, 10:08 AM, "Tom Lane" <t...@sss.pgh.pa.us> wrote: > "Bossart, Nathan" <bossa...@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
