Re: Inconsistent behavior of pg_dump/pg_restore on DEFAULT PRIVILEGES

On 10/18/21, 8:20 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Masahiko Sawada <sawada.mshk@gmail.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