Re: pg_dump handling of ALTER DEFAULT PRIVILEGES IN SCHEMA

On 9/5/21, 10:08 AM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Bossart, Nathan" <bossartn(at)amazon(dot)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