From: Stephen Frost <sfrost@snowman.net>
To: Robert Haas <robertmhaas@gmail.com>
Cc: David Steele <david@pgmasters.net>
Cc: Sawada Masahiko <sawada.mshk@gmail.com>
Cc: Fujii Masao <masao.fujii@gmail.com>
Cc: Simon Riggs <simon@2ndquadrant.com>
Cc: Tatsuo Ishii <ishii@postgresql.org>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Abhijit Menon-Sen <ams@2ndquadrant.com>
Subject: Re: Auditing extension for PostgreSQL (Take 2)
Date: Thu, 14 May 2015 10:46:02 -0400
Message-ID: <20150514144602.GV30322@tamriel.snowman.net> (raw)
In-Reply-To: <CA+TgmobtrK9ndfBeAhui1KDKpnUp4cNv07KukD73jOAfd=4dUg@mail.gmail.com>
References: <CAD21AoCxFOLWk-k179PyFxzwjkOW8oPaNk73GxZi+Yw2+n4YFA@mail.gmail.com>
<553938A1.7030707@pgmasters.net>
<CAD21AoDBt44ABM_0WMketM8bZtqLBMrJpd9RD5tE=Dkr57ft_A@mail.gmail.com>
<553FA477.5010803@pgmasters.net>
<CAD21AoCQSGZ5x+sfs4g1x_8Fip60x8ULQFTAGB5XTrKQZvPWPA@mail.gmail.com>
<CAHGQGwGJnRW69rSL1=tza571_eA44hBeChg-NAVrqQytESgvLg@mail.gmail.com>
<55429DA5.40503@pgmasters.net>
<CAD21AoCRhxrXuFOGLMHNuyfzTCqwHPSKrZWwsw+M8cv_CawFag@mail.gmail.com>
<5551525A.6070709@pgmasters.net>
<CA+TgmobtrK9ndfBeAhui1KDKpnUp4cNv07KukD73jOAfd=4dUg@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, robertmhaas@gmail.com, david@pgmasters.net, sawada.mshk@gmail.com, masao.fujii@gmail.com, simon@2ndquadrant.com, ishii@postgresql.org, ams@2ndquadrant.com
Subject: Re: Auditing extension for PostgreSQL (Take 2)
In-Reply-To: <20150514144602.GV30322@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox