Re: pg_dump dump catalog ACLs

On 04/02/2016 03:23 AM, Stephen Frost wrote: [snip]
>> Patch does not apply cleanly (via "git am") as of today's master; but does apply with "patch"; minor fuzzes apart, so tested with that.
> Just needed to be rebased and then updated to reflect the additions of
> dumping the access methods to pg_dump. Updated patch series attached.

Confirmed clean, thanks! :)

> * Initial ACL preservation:
> " * Note that any initial ACLs (see pg_init_privs) will be removed when we
> * extract the information about the object. We don't provide support for
> * initial policies and security labels and it seems unlikely for those
> * to ever exist, but we may have to revisit this later.
> " ... fair enough, but it ought to be documented in the manual, too. IMO.
> Fixed.

Check.

> DOCUMENTATION
> - Under "privtype" it says: "A code defining the type of initial privilege of this object; see text" ... but the patch doesn't seem to contain the referenced text¿?
> A minor omission, definitively --- it is indeed easily derived from enum InitPrivsType's definition at include/catalog/pg_init_privs.h
> Fixed.

Check

[snip]
>> Within buildACLCommands(), multiple ocurrences of strncmp( ... ,"group ", strlen("group ") could have been avoided, but code simplicity/readability could be affected
>> ... whereas in other places one reads "if( strncmp(nsinfo->dobj.name, "pg_catalog", 10) == 0) "
> Fixed by doing 'strlen("pg_catalog")' in the one spot which had been
> using a hard-coded value of '10'.

Ok. I tend to do ".... ,10 /*strlen("pg_catalog")*/ , ... " instead,
but this is definitively good.

> [snip]
> The new status of this patch is: Ready for Committer
> Very cool.
>
> Thanks again for the review!

Thank you, Stephen

/ J.L.