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