Robert, all, * Robert Haas ([email protected]) wrote: > On Mon, May 11, 2015 at 9:07 PM, David Steele <[email protected]> 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
issues.
> + 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.
Thanks!
Stephen
signature.asc
Description: Digital signature
