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,
sotested 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
patchdoesn'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,
butcode 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.