Date: Thu, 14 May 2015 10:46:02 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Robert Haas <robertmhaas@gmail.com>
Cc: David Steele <david@pgmasters.net>,
 Sawada Masahiko <sawada.mshk@gmail.com>,
 Fujii Masao <masao.fujii@gmail.com>, Simon Riggs <simon@2ndquadrant.com>,
 Tatsuo Ishii <ishii@postgresql.org>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Abhijit Menon-Sen <ams@2ndquadrant.com>
Subject: Re: Auditing extension for PostgreSQL (Take 2)
Message-ID: <20150514144602.GV30322@tamriel.snowman.net>
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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature"; boundary="rHMJWoIx6y761zdp"
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmobtrK9ndfBeAhui1KDKpnUp4cNv07KukD73jOAfd=4dUg@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--rHMJWoIx6y761zdp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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 =3D=3D T_DoStmt &&
>=20
> 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:
>=20
> +       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

--rHMJWoIx6y761zdp
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJVVLUpAAoJEO1sijiDR2RV4pIP/3CGPriALkUJbWu84GsQL7cx
PcrjFeyD12qEtxGoI8MpXEmtIREUXAvTXX67E/3+VrxiXNIaMRLsaMEN6O2le3hC
A0+/M/lL0KU372ks0dyhR+47aF1LCMLnSsW2bwl1LoOoCzHnletbZ5EuEoOGiRgW
ugY/9rjnhMGDCSXRiGin/DiArXYUJSaPclMCUSW0DWkARC8a5LVA+eeM9fRQ/5LL
73b6275Upt2MdxJz/6hX3nw2U1uleeeA7ETEMyMM8Zpg3hPehHEE+D/t5iout5vj
osXjoOHw3gWdSV1iXgzIQckJMvF/IzsdPta+f+rl8fQV5ho/POWPk1misK4GcqeF
B8MW82e0qQowSWzyUX/22DQwYNRRUOQZkyiIne0Yr7rLdPHJxVGJpwxQOFCeQJQg
fmHo+sKX3WWcoeppQtevqODoJ20qKkgze57C1HnXfA4d+7eBHiLy/x/fqwgJQAqD
hsSFwSQTyLSZr62ng0FHVVLdcQQ402mLV5oAXwkGPdhLV3pB3vbOfg0Fo+KehsRl
eLlNjcL5x5+ktiobZ7/Ml5kVhRVkiVjq9dKPwehewm55VDbphDMD2VrmGJGII7yT
h+6heWsOHUIHxz2VXT4lT65kDz1V17DncIfxLKIwEkR95j1cKpptgIYrzlCR7M0n
3w+eOnhXWkRwN6QUVDZF
=BktN
-----END PGP SIGNATURE-----

--rHMJWoIx6y761zdp--