Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

On 10/18/21, 8:20 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
>> I've looked at the patch proposed you proposed. If we can depend on
>> acldefault() being STRICT (which is legitimate to me), I think we
>> don't need to build an expression depending on the caller (i.g.,
>> is_default_acl). If acldefault() were to become not STRICT, we could
>> detect it by regression tests. What do you think?
>
> FWIW, I'm working on a refactoring of this logic that will bring the
> acldefault() call into the getDefaultACLs code, which would mean that
> we won't need that assumption anymore anyway. The code as I have it
> produces SQL like

Nice!

> So I think we don't need to worry about whether acldefault() will stay
> strict. This patch will only need to work in the back branches.

This seems fine to me, too. I don't think relying on STRICT is any
better or worse than adding a flag for this one special case.

+ /*
+ * Since the default for a global entry is the hard-wired default
+ * ACL for the particular object type, we pass defaclobjtype except
+ * for the case of 'S' (DEFACLOBJ_SEQUENCE) where we need to
+ * transform it to 's' since acldefault() SQL-callable function
+ * handles 's' as a sequence. On the other hand, since the default
+ * for non-global entries is an empty ACL we pass NULL. This works
+ * because acldefault() is STRICT.
+ */

I'd split out the two special cases in the comment. What do you think
about something like the following?

/*
* Build the expression for determining the object type.
*
* While pg_default_acl uses 'S' for sequences, acldefault()
* uses 's', so we must transform 'S' to 's'.
*
* The default for a schema-local default ACL (i.e., entries
* in pg_default_acl with defaclnamespace != 0) is an empty
* ACL. We use NULL as the object type for those entries,
* which forces acldefault() to also return NULL because it is
* STRICT.
*/

+ create_sql => 'ALTER DEFAULT PRIVILEGES
+ FOR ROLE regress_dump_test_role IN SCHEMA dump_test
+ GRANT EXECUTE ON FUNCTIONS TO regress_dump_test_role;',
+ regexp => qr/^
+ \QALTER DEFAULT PRIVILEGES \E
+ \QFOR ROLE regress_dump_test_role IN SCHEMA dump_test \E
+ \QGRANT ALL ON FUNCTIONS TO regress_dump_test_role;\E
+ /xm,

It could be a bit confusing that create_sql uses "GRANT EXECUTE" but
we expect to see "GRANT ALL." IIUC this is correct, but maybe we
should use "GRANT ALL" in create_sql so that they match.

Nathan