Re: pg_dump dump catalog ACLs

From: Jose Luis Tallon <jltallon(at)adv-solutions(dot)net>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_dump dump catalog ACLs
Date: 2016-03-30 17:35:02
Message-ID: 20160330173502.1315.17147.pgcf@coridan.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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 tested with that.
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)

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

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

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)

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 "dumping required"
(i.e. not indent the dumpXXX after the "if ( yyy->dobj.dump & ... )"
After all, pgindent will take care of tabs before release :)

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) "

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.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message José Luis Tallón 2016-03-30 17:41:30 [CommitFest App] Feature request -- review e-mail additions
Previous Message Josh berkus 2016-03-30 17:01:18 Re: Desirable pgbench features?