Re: pg_dump dump catalog ACLs - Mailing list pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Patch does not apply cleanly (via "git am") as of today's master; but does apply with "patch"; minor fuzzes apart, so
testedwith that.
 
The functionality contained in this patch was badly needed: IMHO, it is a BUG that we do not *perfectly* restore (even
whenit's only default and catalog/extensions)
 

DESIGN/DOCUMENTATION
* int4 for the "objsubid" field? and int2 would surely suffice, given that we only allow up to 1600 columns ... if it's
amatter of alignment, it would be interesting to say so somewhere (code comments, maybe?)
 
* Initial ACL preservation:
"       * Note that any initial ACLs (see pg_init_privs) will be removed when we       * extract the information about
theobject.  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.


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

CODE
"#if ACLDEBUG" code blocks are still present; should be removed before commiting  (I quite like the DumpComponents
typedefvs bool [previous], BTW; Clean, readable and effective)
 

Code comments are descriptive enough and generally properly spellt. Exception:  src/bin/pg_dump/dumputils.c:
s/privilegs/privileges/ (line 132)
 

Minor issue: diff size could have been really reduced by omitting the extra indent after the newly-included checks for
"dumpingrequired"   (i.e. not indent the dumpXXX after the "if ( yyy->dobj.dump & ... )"   After all, pgindent will
takecare of tabs before release :)
 

Within buildACLCommands(), multiple ocurrences of strncmp( ... ,"group ", strlen("group ") could have been avoided, but
codesimplicity/readability could be affected... whereas in other places one reads "if( strncmp(nsinfo->dobj.name,
"pg_catalog",10) == 0) "
 

I think it is debatable whether DumpOptions belong inside Archive or not (I would have kept it separate, FWIW), but
thatis unrelated to the correctness of this patch.
 

Code builds, installs and "make check"s ok.

TESTING:- pg_dumpall properly dumps the REVOKE statements to store the ACL changes to catalog objects- pg_init_privs
recordsall initial privileges on objects (from initdb time)- "psql -f" re-creates all privileges 

The new status of this patch is: Ready for Committer