|From:||Stephen Frost <sfrost(at)snowman(dot)net>|
|To:||Robert Haas <robertmhaas(at)gmail(dot)com>|
|Cc:||David Steele <david(at)pgmasters(dot)net>, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Tatsuo Ishii <ishii(at)postgresql(dot)org>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>|
|Subject:||Re: Auditing extension for PostgreSQL (Take 2)|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, May 11, 2015 at 9:07 PM, David Steele <david(at)pgmasters(dot)net> wrote:
> > The attached v12 patch removes the code that became redundant with
> > Alvaro committing the event trigger/deparse work. I've updated the
> > regression tests to reflect the changes, which were fairly minor and add
> > additional information to the output. There are no longer any #ifdef'd
> > code blocks.
> This is not a full review, but just a few thoughts...
Thanks for that. David and I worked through your suggestions, a number
of my own, and some general cleanup and I've now pushed it.
> What happens if the server crashes? Presumably, audit records emitted
> just before the crash can be lost, possibly even if the transaction
> went on to commit. That's no worse than what is already the case for
> regular logging, of course, but it's maybe a bit more relevant here
> because of the intended use of this information.
Right, if the server crashes then we may lose information- but there
should be a log somewhere of the crash. I didn't do much in the way of
changes to the documentation, but this is definitely an area where we
should make it very clear what happens.
> Braces around single-statement blocks are not PostgreSQL style.
Fixed those and a number of other things, like not having entire
functions in if() blocks.
> I wonder if driving the auditing system off of the required
> permissions is really going to work well. That means that decisions
> we make about permissions enforcement will have knock-on effects on
> auditing. For example, the fact that we check permission on a view
> rather than on the underlying tables will (I think) flow through to
> how the auditing happens.
The checks against the permissions are independent and don't go through
our normal permission checking system, so I'm not too worried about this
aspect. I agree that we need to be vigilant and consider the impact of
changes to the permissions system, but there are also quite a few
regression tests in pg_audit and those should catch a lot of potential
> + stackItem->auditEvent.commandTag == T_DoStmt &&
> Use IsA(..., DoStmt) for this kind of test. There are many instances
> of this pattern in the patch; they should al be fixed.
Unfortunately, that's not actually a Node, so we can't just use IsA. We
considered making it one, but that would mean IsA() would return a
T_DoStmt or similar for something that isn't actually a T_DoStmt (it's
an auditEvent of a T_DoStmt). Still, I did go through and look at these
cases and made changes to improve them and clean things up to be neater.
> The documentation and comments in this patch are, by my estimation,
> somewhat below our usual standard. For example:
> + DefineCustomBoolVariable("pg_audit.log_notice",
> + "Raise a
> notice when logging",
This was improved, but I'm sure more can be done. Suggestions welcome.
> Granted, there is a fuller explanation in the documentation, but that
> doesn't make that explanation particularly good. (One might also
> wonder why the log level isn't fully configurable instead of offering
> only two choices.)
This was certainly a good point and we added support for choosing the
log level to log it at.
> I don't want to focus too much on this particular example. The
> comments and documentation really deserve a bit more attention
> generally than they have gotten thus far. I am not saying they are
> bad. I am just saying that, IMHO, they are not as good as we
> typically try to make them.
I've done quite a bit of rework of the comments and will be working on
improving the documentation also.
|Next Message||Stephen Frost||2015-05-14 14:48:33||Re: pgsql: Add pg_audit, an auditing extension|
|Previous Message||Stephen Frost||2015-05-14 14:36:41||pgsql: Add pg_audit, an auditing extension|