Re: pg_dump dump catalog ACLs

Jose,

* Jose Luis Tallon (jltallon(at)adv-solutions(dot)net) wrote:
> 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

Thanks for the review!

> 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.

> The functionality contained in this patch was badly needed: IMHO, it is a BUG that we do not *perfectly* restore (even when it's only default and catalog/extensions)

Guess I have to agree with that. :)

> DESIGN/DOCUMENTATION
> * int4 for the "objsubid" field? and int2 would surely suffice, given that we only allow up to 1600 columns ... if it's a matter of alignment, it would be interesting to say so somewhere (code comments, maybe?)

As Robert noted, this is a long-standing convention and is used
throughout our existing code-base. It's certainly not the purview of
this particular patch to change that.

> * 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.

> 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.

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

The '#if ACLDEBUG's are not from this patch-set and I don't think it's
this patch's job to remove them ...

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

Fixed.

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

Fixed as best as I could, pgindent is pretty clean for the changes which
this patch set introduces (note that other patches have been committed
which are not pgindent clean and therefore certain files will still be
changed on the next pgindent run, such as pg_dump.c).

> 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'.

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

I didn't see the sense of changing that as part 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 records all initial privileges on objects (from initdb time)
> - "psql -f" re-creates all privileges
>
> The new status of this patch is: Ready for Committer

Very cool.

Thanks again for the review!

I invite others to take a look at the new catalog ('pg_init_privs') and
the changes made to pg_dump and voice any concerns or opinions regarding
those or anything else in this patch set as I've now been through it
again, cleaned up and added more comments, cleaned up and added more to
the documentation, even found a couple bugs which I fixed (particularly
around populating pg_init_privs for columns, and in buildACLCommands()
in pg_dump which is a bit tricky, but now has a lot more comments) and
generally am feeling pretty happy about this patch set.

Barring objections, I'm planning to push this in the next few days.

Thanks!

Stephen