Re: Auditing extension for PostgreSQL (Take 2) - Mailing list pgsql-hackers

Robert, all,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, May 11, 2015 at 9:07 PM, David Steele <david@pgmasters.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
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