Обсуждение: pgsql: Add pg_audit, an auditing extension

Add pg_audit, an auditing extension

This extension provides detailed logging classes, ability to control
logging at a per-object level, and includes fully-qualified object
names for logged statements (DML and DDL) in independent fields of the
log output.

Authors: Ian Barwick, Abhijit Menon-Sen, David Steele
Reviews by: Robert Haas, Tatsuo Ishii, Sawada Masahiko, Fujii Masao,
Simon Riggs

Discussion with: Josh Berkus, Jaime Casanova, Peter Eisentraut,
David Fetter, Yeb Havinga, Alvaro Herrera, Petr Jelinek, Tom Lane,
MauMau, Bruce Momjian, Jim Nasby, Michael Paquier,
Fabrízio de Royes Mello, Neil Tiffin

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/ac52bb0442f79076b14acd8ad5b696946c1053b8

Modified Files
--------------
contrib/Makefile                       |    1 +
contrib/pg_audit/.gitignore            |    5 +
contrib/pg_audit/Makefile              |   21 +
contrib/pg_audit/expected/pg_audit.out |  964 ++++++++++++++++
contrib/pg_audit/pg_audit--1.0.0.sql   |   22 +
contrib/pg_audit/pg_audit.c            | 1870 ++++++++++++++++++++++++++++++++
contrib/pg_audit/pg_audit.conf         |    1 +
contrib/pg_audit/pg_audit.control      |    5 +
contrib/pg_audit/sql/pg_audit.sql      |  617 +++++++++++
doc/src/sgml/contrib.sgml              |    1 +
doc/src/sgml/filelist.sgml             |    1 +
doc/src/sgml/pgaudit.sgml              |  678 ++++++++++++
12 files changed, 4186 insertions(+)

* Stephen Frost (sfrost@snowman.net) wrote:
> Add pg_audit, an auditing extension

And... I busted the buildfarm.  Will fix.

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> And... I busted the buildfarm.  Will fix.

 -- Load pg_audit module
  create extension pg_audit;
+ ERROR:  pg_audit must be loaded via shared_preload_libraries

This seems like a rather poorly thought-through error check.
It will break not only the buildfarm but any dump/restore scenario.
You really can't have extensions that refuse to let themselves
be created.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > And... I busted the buildfarm.  Will fix.
>
>  -- Load pg_audit module
>   create extension pg_audit;
> + ERROR:  pg_audit must be loaded via shared_preload_libraries
>
> This seems like a rather poorly thought-through error check.
> It will break not only the buildfarm but any dump/restore scenario.
> You really can't have extensions that refuse to let themselves
> be created.

Yeah, the original idea behind it was to force the user to think about
if they really would want to load it later on down the line rather than
have it pre-loaded always.

I'll put something in the docs which recommends it and provides the
reasoning behind it.

    Thanks!

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> + ERROR:  pg_audit must be loaded via shared_preload_libraries
>>
>> This seems like a rather poorly thought-through error check.
>> It will break not only the buildfarm but any dump/restore scenario.
>> You really can't have extensions that refuse to let themselves
>> be created.

> Yeah, the original idea behind it was to force the user to think about
> if they really would want to load it later on down the line rather than
> have it pre-loaded always.

> I'll put something in the docs which recommends it and provides the
> reasoning behind it.

Could we apply a check at some later time, when the user actually does
something that is not sensible unless the library was preloaded?  Even
then, just a WARNING might be better than ERROR.

(Still, it's not clear how you'd get buildfarm testing to pass, so
maybe this line of thought is just as fruitless.)

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> + ERROR:  pg_audit must be loaded via shared_preload_libraries
> >>
> >> This seems like a rather poorly thought-through error check.
> >> It will break not only the buildfarm but any dump/restore scenario.
> >> You really can't have extensions that refuse to let themselves
> >> be created.
>
> > Yeah, the original idea behind it was to force the user to think about
> > if they really would want to load it later on down the line rather than
> > have it pre-loaded always.
>
> > I'll put something in the docs which recommends it and provides the
> > reasoning behind it.
>
> Could we apply a check at some later time, when the user actually does
> something that is not sensible unless the library was preloaded?  Even
> then, just a WARNING might be better than ERROR.

Actually, we do..  The order in which the hooks are called matters and
if it's not loaded before anything real is called then it's going to
blow up.

> (Still, it's not clear how you'd get buildfarm testing to pass, so
> maybe this line of thought is just as fruitless.)

I've pushed a change which should clean it up by simply loading the
module after each reconnects is done, more-or-less simulating having it
be in shared_preload_libraries.  It also wasn't using the correct
database for reconnecting.

I'll keep an eye on it.

    Thanks!

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I've pushed a change which should clean it up by simply loading the
> module after each reconnects is done, more-or-less simulating having it
> be in shared_preload_libraries.  It also wasn't using the correct
> database for reconnecting.

> I'll keep an eye on it.

Another thing that looks not amazingly well-thought-out about that
regression test is that it creates a superuser role with a known name
(and no password, not that adding a password would make it better).
This seems like it's just asking for trouble, especially in installcheck
scenarios where failure partway through would leave the superuser lying
around ready to be exploited.  Do we *really* need that in the test?

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > I've pushed a change which should clean it up by simply loading the
> > module after each reconnects is done, more-or-less simulating having it
> > be in shared_preload_libraries.  It also wasn't using the correct
> > database for reconnecting.
>
> > I'll keep an eye on it.
>
> Another thing that looks not amazingly well-thought-out about that
> regression test is that it creates a superuser role with a known name
> (and no password, not that adding a password would make it better).
> This seems like it's just asking for trouble, especially in installcheck
> scenarios where failure partway through would leave the superuser lying
> around ready to be exploited.  Do we *really* need that in the test?

I'm not sure that we need it..  It's certainly darn handy to have since
we're reconnecting to get the role attributes updated when the GUCs are
being changed and we need to be doing that as a superuser.  We certainly
create superuser login roles throughout the regression tests today,
including those in core (CREATE ROLE foreign_data_user LOGIN SUPERUSER;
for example).  I had thought the issues with having a superuser role in
the regression tests had been addressed by not allowing the socket to be
in a public spot and the other changes..

We don't currently drop the role though at the end though.

If we had the name of the role to reconnect as then I don't think we'd
need the "super" role, but we'd need to be able to reconnect as that
role too, to be able to flip back and forth between roles which are not
superusers and ones which are.

Will investigate that, suggestions certainly welcome.  Looks like
there's a more interesting issue that termite ran into.

    Thanks!

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> We don't currently drop the role though at the end though.

Quite aside from any security risks, that means that running "make
installcheck" twice in a row fails.  Please fix.

> Will investigate that, suggestions certainly welcome.  Looks like
> there's a more interesting issue that termite ran into.

Yeah, lapwing too.  If it reproduces on any of my critters, I'll
take a look.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > We don't currently drop the role though at the end though.
>
> Quite aside from any security risks, that means that running "make
> installcheck" twice in a row fails.  Please fix.

Right, will do, though one kind of requires the other (we can't drop the
only user we know how to connect as which is a superuser...).  I'll
figure out a way to make it work though.

> > Will investigate that, suggestions certainly welcome.  Looks like
> > there's a more interesting issue that termite ran into.
>
> Yeah, lapwing too.  If it reproduces on any of my critters, I'll
> take a look.

That'd be great, I'm trying to reproduce it here but I'm not seeing it.

I'm at least somewhat suspicious it has to do with loading the library,
since we (pretty clearly..) didn't test that much as it's not really
intended.

    Thanks!

        Stephen

On 14 May 2015 at 15:36, Stephen Frost <sfrost@snowman.net> wrote:
> Add pg_audit, an auditing extension
>
> This extension provides detailed logging classes, ability to control
> logging at a per-object level, and includes fully-qualified object
> names for logged statements (DML and DDL) in independent fields of the
> log output.
>
> Authors: Ian Barwick, Abhijit Menon-Sen, David Steele
> Reviews by: Robert Haas, Tatsuo Ishii, Sawada Masahiko, Fujii Masao,
> Simon Riggs
>
> Discussion with: Josh Berkus, Jaime Casanova, Peter Eisentraut,
> David Fetter, Yeb Havinga, Alvaro Herrera, Petr Jelinek, Tom Lane,
> MauMau, Bruce Momjian, Jim Nasby, Michael Paquier,
> Fabrízio de Royes Mello, Neil Tiffin

A quick glance shows a parameter called pg_audit.log_statement_once,
which isn't mentioned in the docs.  Also, pg_audit.log_level isn't in
the logs, but seems to offer a superset of functionality to
pg_audit.log_notice, which doesn't appear as a valid config parameter.

Is this expected?  Are the doc updates supposed to upcoming anyway?

--
Thom

Thom,

* Thom Brown (thom@linux.com) wrote:
> A quick glance shows a parameter called pg_audit.log_statement_once,
> which isn't mentioned in the docs.  Also, pg_audit.log_level isn't in
> the logs, but seems to offer a superset of functionality to
> pg_audit.log_notice, which doesn't appear as a valid config parameter.

> Is this expected?  Are the doc updates supposed to upcoming anyway?

Thanks!  Yes, doc updates are coming and I'll make sure to include
those.

    Thanks again,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Quite aside from any security risks, that means that running "make
>> installcheck" twice in a row fails.  Please fix.

> Right, will do, though one kind of requires the other (we can't drop the
> only user we know how to connect as which is a superuser...).  I'll
> figure out a way to make it work though.

Instead of physically reconnecting, could you do SET ROLE or SET SESSION
AUTHORIZATION?  I think that's what we do in the core tests.

> I'm at least somewhat suspicious it has to do with loading the library,
> since we (pretty clearly..) didn't test that much as it's not really
> intended.

There are several more crashes in the BF now.  They're not at initial
library load AFAICS.  Hard to tell if it's platform-specific or just
randomly fails sometimes.  Have you tried valgrind to see if there's
uninitialized-memory touches?

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Quite aside from any security risks, that means that running "make
> >> installcheck" twice in a row fails.  Please fix.
>
> > Right, will do, though one kind of requires the other (we can't drop the
> > only user we know how to connect as which is a superuser...).  I'll
> > figure out a way to make it work though.
>
> Instead of physically reconnecting, could you do SET ROLE or SET SESSION
> AUTHORIZATION?  I think that's what we do in the core tests.

Unfortunately, neither of those reset the role GUCs which are set with
ALTER ROLE.

> > I'm at least somewhat suspicious it has to do with loading the library,
> > since we (pretty clearly..) didn't test that much as it's not really
> > intended.
>
> There are several more crashes in the BF now.  They're not at initial
> library load AFAICS.  Hard to tell if it's platform-specific or just
> randomly fails sometimes.  Have you tried valgrind to see if there's
> uninitialized-memory touches?

Will give that a shot and see what it says.

    Thanks!

        Stephen

Thom,

* Thom Brown (thom@linux.com) wrote:
> On 14 May 2015 at 15:36, Stephen Frost <sfrost@snowman.net> wrote:
> > Add pg_audit, an auditing extension
>
> A quick glance shows a parameter called pg_audit.log_statement_once,
> which isn't mentioned in the docs.  Also, pg_audit.log_level isn't in
> the logs, but seems to offer a superset of functionality to
> pg_audit.log_notice, which doesn't appear as a valid config parameter.
>
> Is this expected?  Are the doc updates supposed to upcoming anyway?

Actually, I see them both in pgaudit.sgml in master..?

Is there somewhere else they're missing from?

    Thanks!

        Stephen

On 14 May 2015 at 17:45, Stephen Frost <sfrost@snowman.net> wrote:
> Thom,
>
> * Thom Brown (thom@linux.com) wrote:
>> On 14 May 2015 at 15:36, Stephen Frost <sfrost@snowman.net> wrote:
>> > Add pg_audit, an auditing extension
>>
>> A quick glance shows a parameter called pg_audit.log_statement_once,
>> which isn't mentioned in the docs.  Also, pg_audit.log_level isn't in
>> the logs, but seems to offer a superset of functionality to
>> pg_audit.log_notice, which doesn't appear as a valid config parameter.
>>
>> Is this expected?  Are the doc updates supposed to upcoming anyway?
>
> Actually, I see them both in pgaudit.sgml in master..?
>
> Is there somewhere else they're missing from?

Ah, my apologies.  I can see from the file date that pgaudit.html is
dated 2 weeks ago, so I'm looking at an old copy.  Not sure what
happened there as all other files were newer.  In any case, I've
rebuilt and see neither of those issues I reported exist anymore.
Sorry for the noise.

--
Thom

Thom,

* Thom Brown (thom@linux.com) wrote:
> On 14 May 2015 at 17:45, Stephen Frost <sfrost@snowman.net> wrote:
> > Is there somewhere else they're missing from?
>
> Ah, my apologies.  I can see from the file date that pgaudit.html is
> dated 2 weeks ago, so I'm looking at an old copy.  Not sure what
> happened there as all other files were newer.  In any case, I've
> rebuilt and see neither of those issues I reported exist anymore.
> Sorry for the noise.

No problem at all, please let me know if you come across any issues.

    Thanks!

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> There are several more crashes in the BF now.  They're not at initial
>> library load AFAICS.  Hard to tell if it's platform-specific or just
>> randomly fails sometimes.  Have you tried valgrind to see if there's
>> uninitialized-memory touches?

> Will give that a shot and see what it says.

At least on dromedary, this seems to be the problem:

pg_audit.c: In function 'stack_pop':
pg_audit.c:387: warning: format '%ld' expects type 'long int', but argument 3 has type 'int64'
pg_audit.c: In function 'stack_valid':
pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument 3 has type 'int64'
pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument 4 has type 'int64'
pg_audit.c: In function 'log_audit_event':
pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument 4 has type 'int64'
pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument 5 has type 'int64'

Will push a fix shortly and we'll see what happens.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> There are several more crashes in the BF now.  They're not at initial
> >> library load AFAICS.  Hard to tell if it's platform-specific or just
> >> randomly fails sometimes.  Have you tried valgrind to see if there's
> >> uninitialized-memory touches?
>
> > Will give that a shot and see what it says.
>
> At least on dromedary, this seems to be the problem:
>
> pg_audit.c: In function 'stack_pop':
> pg_audit.c:387: warning: format '%ld' expects type 'long int', but argument 3 has type 'int64'
> pg_audit.c: In function 'stack_valid':
> pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument 3 has type 'int64'
> pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument 4 has type 'int64'
> pg_audit.c: In function 'log_audit_event':
> pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument 4 has type 'int64'
> pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument 5 has type 'int64'
>
> Will push a fix shortly and we'll see what happens.

Ah, ok.

    Thanks!

        Stephen

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Quite aside from any security risks, that means that running "make
> >> installcheck" twice in a row fails.  Please fix.
>
> > Right, will do, though one kind of requires the other (we can't drop the
> > only user we know how to connect as which is a superuser...).  I'll
> > figure out a way to make it work though.
>
> Instead of physically reconnecting, could you do SET ROLE or SET SESSION
> AUTHORIZATION?  I think that's what we do in the core tests.

Alright, I believe this has been fixed now, using the brand-new \gset
option.

Two installcheck's in a row still breaks though..  I'm not quite sure
what to do about that but I'm certainly open to thoughts.  I can reset
the role attributes later, but those get logged with the username used
too in the ALTER statement, which changes.

I'll continue to think about it though, perhaps there's a way I can
disable logging as the superuser without it logging the role involved.

    Thanks!

        Stephen

* Stephen Frost (sfrost@snowman.net) wrote:
> I'll continue to think about it though, perhaps there's a way I can
> disable logging as the superuser without it logging the role involved.

Of course, it occured to me how to address this immediately after, even
though it hadn't in the hour or so prior.  I can just bump
client_min_messages up to warning and then reset the role attributes...

That appears to be working.  Will push an update to fix this shortly.

    Thanks!

        Stephen

On 15 May 2015 at 10:56, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
> I am testing it, and output is strange
>
> 2015-05-15 11:49:25.046 CEST pavel postgres: LOG:  AUDIT:
> SESSION,1,1,DDL,DROP TABLE,,,drop table foo;,<not logged>
> 2015-05-15 11:49:25.046 CEST pavel postgres: STATEMENT:  drop table foo;
> 2015-05-15 11:49:28.291 CEST pavel postgres: LOG:  AUDIT:
> SESSION,2,1,DDL,CREATE TABLE,,,"CREATE TABLE foo(a int, b int);",<not
> logged>
> 2015-05-15 11:49:28.291 CEST pavel postgres: STATEMENT:  CREATE TABLE foo(a
> int, b int);
> 2015-05-15 11:49:31.486 CEST pavel postgres: LOG:  AUDIT:
> SESSION,3,1,WRITE,INSERT,,,"INSERT INTO foo VALUES(10,20);",<not logged>
> 2015-05-15 11:49:31.486 CEST pavel postgres: STATEMENT:  INSERT INTO foo
> VALUES(10,20);
> 2015-05-15 11:49:33.446 CEST pavel postgres: LOG:  AUDIT:
> SESSION,4,1,READ,SELECT,,,SELECT * FROM foo WHERE a = 10;,<not logged>
> 2015-05-15 11:49:33.446 CEST pavel postgres: STATEMENT:  SELECT * FROM foo
> WHERE a = 10;
>
> I am missing object name, unexpected string <not logged>
>
> configuration:
> pg_audit.log = 'read, write, ddl'

From what I can tell, that last value should be for parameters.  You'd
need to set pg_audit.log_parameter to on, and then prepare and execute
a statement with a parameter.

--
Thom

On 15 May 2015 at 11:20, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-05-15 12:14 GMT+02:00 Thom Brown <thom@linux.com>:
>>
>> On 15 May 2015 at 10:56, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> > Hi
>> >
>> > I am testing it, and output is strange
>> >
>> > 2015-05-15 11:49:25.046 CEST pavel postgres: LOG:  AUDIT:
>> > SESSION,1,1,DDL,DROP TABLE,,,drop table foo;,<not logged>
>> > 2015-05-15 11:49:25.046 CEST pavel postgres: STATEMENT:  drop table foo;
>> > 2015-05-15 11:49:28.291 CEST pavel postgres: LOG:  AUDIT:
>> > SESSION,2,1,DDL,CREATE TABLE,,,"CREATE TABLE foo(a int, b int);",<not
>> > logged>
>> > 2015-05-15 11:49:28.291 CEST pavel postgres: STATEMENT:  CREATE TABLE
>> > foo(a
>> > int, b int);
>> > 2015-05-15 11:49:31.486 CEST pavel postgres: LOG:  AUDIT:
>> > SESSION,3,1,WRITE,INSERT,,,"INSERT INTO foo VALUES(10,20);",<not logged>
>> > 2015-05-15 11:49:31.486 CEST pavel postgres: STATEMENT:  INSERT INTO foo
>> > VALUES(10,20);
>> > 2015-05-15 11:49:33.446 CEST pavel postgres: LOG:  AUDIT:
>> > SESSION,4,1,READ,SELECT,,,SELECT * FROM foo WHERE a = 10;,<not logged>
>> > 2015-05-15 11:49:33.446 CEST pavel postgres: STATEMENT:  SELECT * FROM
>> > foo
>> > WHERE a = 10;
>> >
>> > I am missing object name, unexpected string <not logged>
>> >
>> > configuration:
>> > pg_audit.log = 'read, write, ddl'
>>
>> From what I can tell, that last value should be for parameters.  You'd
>> need to set pg_audit.log_parameter to on, and then prepare and execute
>> a statement with a parameter.
>
>
> yes
>
> 2015-05-15 12:18:39.545 CEST pavel postgres: LOG:  AUDIT:
> SESSION,1,1,READ,PREPARE,,,prepare x(int) as select * from foo where a =
> $1;,<none>
> 2015-05-15 12:18:39.545 CEST pavel postgres: STATEMENT:  prepare x(int) as
> select * from foo where a = $1;
> 2015-05-15 12:18:48.065 CEST pavel postgres: LOG:  AUDIT:
> SESSION,2,1,READ,SELECT,,,prepare x(int) as select * from foo where a =
> $1;,10
> 2015-05-15 12:18:48.065 CEST pavel postgres: STATEMENT:  execute x(10);
>
> but when pg_audit.log_parameter is off, then this value should be empty

If it were empty, it would then be indistinguishable from a statement
executed with a single parameter passed as an empty string.

Speaking of which, I notice that nulls show up as nothing too.  How
does one distinguish between an empty string and a null in the logs?

--
Thom

* Thom Brown (thom@linux.com) wrote:
> If it were empty, it would then be indistinguishable from a statement
> executed with a single parameter passed as an empty string.

NULL, but yes.

> Speaking of which, I notice that nulls show up as nothing too.  How
> does one distinguish between an empty string and a null in the logs?

It should be "", to match how the COPY protocol handles it.  Will take a
look.

    Thanks!

        Stephen

On Thu, May 14, 2015 at 11:36 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Add pg_audit, an auditing extension
>
> This extension provides detailed logging classes, ability to control
> logging at a per-object level, and includes fully-qualified object
> names for logged statements (DML and DDL) in independent fields of the
> log output.
>
> Authors: Ian Barwick, Abhijit Menon-Sen, David Steele
> Reviews by: Robert Haas, Tatsuo Ishii, Sawada Masahiko, Fujii Masao,
> Simon Riggs
>
> Discussion with: Josh Berkus, Jaime Casanova, Peter Eisentraut,
> David Fetter, Yeb Havinga, Alvaro Herrera, Petr Jelinek, Tom Lane,
> MauMau, Bruce Momjian, Jim Nasby, Michael Paquier,
> Fabrízio de Royes Mello, Neil Tiffin
>
> Branch
> ------
> master
>
> Details
> -------
> http://git.postgresql.org/pg/commitdiff/ac52bb0442f79076b14acd8ad5b696946c1053b8
>
> Modified Files
> --------------
> contrib/Makefile                       |    1 +
> contrib/pg_audit/.gitignore            |    5 +
> contrib/pg_audit/Makefile              |   21 +
> contrib/pg_audit/expected/pg_audit.out |  964 ++++++++++++++++
> contrib/pg_audit/pg_audit--1.0.0.sql   |   22 +
> contrib/pg_audit/pg_audit.c            | 1870 ++++++++++++++++++++++++++++++++
> contrib/pg_audit/pg_audit.conf         |    1 +
> contrib/pg_audit/pg_audit.control      |    5 +
> contrib/pg_audit/sql/pg_audit.sql      |  617 +++++++++++
> doc/src/sgml/contrib.sgml              |    1 +
> doc/src/sgml/filelist.sgml             |    1 +
> doc/src/sgml/pgaudit.sgml              |  678 ++++++++++++
> 12 files changed, 4186 insertions(+)

pg_audit uses 1.0.0 as its version number. But, is the third digit really
required? Why? We usually uses the version number with two digits in
contrib modules.

CREATE EXTENSION pg_audit failed with the following error message
when shared_preload_libraries and pg_audit.log are set to pg_audit and
ddl, respectively.

=# create extension pg_audit ;
ERROR:  pg_event_trigger_ddl_commands() can only be called in an event
trigger function
CONTEXT:  SQL statement "SELECT UPPER(object_type), object_identity
  FROM pg_event_trigger_ddl_commands()"

In Makefile, PGFILEDESC should be added.

+# pg_audit/Makefile

should be "# contrib/pg_audit/Makefile" for the consistency.

The categories of some SQL commands are different between log_statement and
pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
log_statement. That's confusing. We should use the same "category-mapping"
rule as much as possible.

Regards,

--
Fujii Masao

Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:
> pg_audit uses 1.0.0 as its version number. But, is the third digit really
> required? Why? We usually uses the version number with two digits in
> contrib modules.

I have to admit, I didn't look closely at how we handled versions in
contrib modules and that has been the same since the patch was first
posted, as I recall.  No problem changing it to 1.0 and I'll take care
of that soon.

> CREATE EXTENSION pg_audit failed with the following error message
> when shared_preload_libraries and pg_audit.log are set to pg_audit and
> ddl, respectively.
>
> =# create extension pg_audit ;
> ERROR:  pg_event_trigger_ddl_commands() can only be called in an event
> trigger function
> CONTEXT:  SQL statement "SELECT UPPER(object_type), object_identity
>   FROM pg_event_trigger_ddl_commands()"

Interesting.  I'm very curious about this error and if it impacts other
extensions which use event triggers.  I'll look into it.

> In Makefile, PGFILEDESC should be added.
>
> +# pg_audit/Makefile
>
> should be "# contrib/pg_audit/Makefile" for the consistency.

Good points, will address.

> The categories of some SQL commands are different between log_statement and
> pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
> log_statement. That's confusing. We should use the same "category-mapping"
> rule as much as possible.

David, Simon and I have all considered this at different times and my
recollection is that we all felt that it made sense as DDL because
CLUSTER is DDL (which is actually noted in the comments).  However, you
bring up a good point that classifying it as DDL makes it different from
what the core system does and it'd probably be good to be consistent.

The question here really is- is that the right classification for
REINDEX to have in core?  If so, shouldn't CLUSTER have the same?
Probably a discussion to have on -hackers rather than here.  I'll go
ahead and change it to match what core does.  Then, if core changes to
make it DDL, pg_audit will automatically pick that up and also treat it
as DDL.

    Thanks!

        Stephen

Stephen Frost wrote:

> * Fujii Masao (masao.fujii@gmail.com) wrote:

> > CREATE EXTENSION pg_audit failed with the following error message
> > when shared_preload_libraries and pg_audit.log are set to pg_audit and
> > ddl, respectively.
> >
> > =# create extension pg_audit ;
> > ERROR:  pg_event_trigger_ddl_commands() can only be called in an event
> > trigger function
> > CONTEXT:  SQL statement "SELECT UPPER(object_type), object_identity
> >   FROM pg_event_trigger_ddl_commands()"
>
> Interesting.  I'm very curious about this error and if it impacts other
> extensions which use event triggers.  I'll look into it.

Can you explain what's going on here?  It seems a bit odd to me.

> > The categories of some SQL commands are different between log_statement and
> > pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
> > log_statement. That's confusing. We should use the same "category-mapping"
> > rule as much as possible.
>
> David, Simon and I have all considered this at different times and my
> recollection is that we all felt that it made sense as DDL because
> CLUSTER is DDL (which is actually noted in the comments).  However, you
> bring up a good point that classifying it as DDL makes it different from
> what the core system does and it'd probably be good to be consistent.
>
> The question here really is- is that the right classification for
> REINDEX to have in core?  If so, shouldn't CLUSTER have the same?

Thiking a bit about this, it seems that REINDEX and CLUSTER are not
really DDL.  They are more "maintenance commands"; they don't define
anything that wasn't defined before.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/15/15 3:44 PM, Stephen Frost wrote:
> * Fujii Masao (masao.fujii@gmail.com) wrote:
>> CREATE EXTENSION pg_audit failed with the following error message
>> when shared_preload_libraries and pg_audit.log are set to pg_audit and
>> ddl, respectively.
>>
>> =# create extension pg_audit ;
>> ERROR:  pg_event_trigger_ddl_commands() can only be called in an event
>> trigger function
>> CONTEXT:  SQL statement "SELECT UPPER(object_type), object_identity
>>   FROM pg_event_trigger_ddl_commands()"
> Interesting.  I'm very curious about this error and if it impacts other
> extensions which use event triggers.  I'll look into it.

I'm looking into this. There's definitely something strange going on
here, but not sure it's in pg_audit.

>> The categories of some SQL commands are different between log_statement and
>> pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
>> log_statement. That's confusing. We should use the same "category-mapping"
>> rule as much as possible.
> David, Simon and I have all considered this at different times and my
> recollection is that we all felt that it made sense as DDL because
> CLUSTER is DDL (which is actually noted in the comments).  However, you
> bring up a good point that classifying it as DDL makes it different from
> what the core system does and it'd probably be good to be consistent.
>
> The question here really is- is that the right classification for
> REINDEX to have in core?  If so, shouldn't CLUSTER have the same?
> Probably a discussion to have on -hackers rather than here.  I'll go
> ahead and change it to match what core does.  Then, if core changes to
> make it DDL, pg_audit will automatically pick that up and also treat it
> as DDL.
I admit I was influenced by this comment in core:

        case T_ReindexStmt:
            lev = LOGSTMT_ALL;    /* should this be DDL? */
            break;

and the fact that CLUSTER is DDL.  I'm not married to REINDEX as DDL,
though.  I'm happy to make it consistent with core.  If it changes in
core, it would also change in pg_audit.

--
- David Steele
david@pgmasters.net

On Sat, May 16, 2015 at 4:44 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Fujii,
>
> * Fujii Masao (masao.fujii@gmail.com) wrote:
>> pg_audit uses 1.0.0 as its version number. But, is the third digit really
>> required? Why? We usually uses the version number with two digits in
>> contrib modules.
>
> I have to admit, I didn't look closely at how we handled versions in
> contrib modules and that has been the same since the patch was first
> posted, as I recall.  No problem changing it to 1.0 and I'll take care
> of that soon.
>
>> CREATE EXTENSION pg_audit failed with the following error message
>> when shared_preload_libraries and pg_audit.log are set to pg_audit and
>> ddl, respectively.
>>
>> =# create extension pg_audit ;
>> ERROR:  pg_event_trigger_ddl_commands() can only be called in an event
>> trigger function
>> CONTEXT:  SQL statement "SELECT UPPER(object_type), object_identity
>>   FROM pg_event_trigger_ddl_commands()"
>
> Interesting.  I'm very curious about this error and if it impacts other
> extensions which use event triggers.  I'll look into it.
>
>> In Makefile, PGFILEDESC should be added.
>>
>> +# pg_audit/Makefile
>>
>> should be "# contrib/pg_audit/Makefile" for the consistency.
>
> Good points, will address.

And on top of that the following things should be changed:
- Removal of REGRESS_OPTS which is empty
- Removal of MODULE, which overlaps with MODULE_big
- $(WIN32RES) needs to be added to OBJS for Windows versioning
Please find in the patch attached the fixes needed.
--
Michael

Fujii, Michael,

* Fujii Masao (masao.fujii@gmail.com) wrote:
> pg_audit uses 1.0.0 as its version number. But, is the third digit really
> required? Why? We usually uses the version number with two digits in
> contrib modules.

[...]

> In Makefile, PGFILEDESC should be added.
>
> +# pg_audit/Makefile
>
> should be "# contrib/pg_audit/Makefile" for the consistency.
>
> The categories of some SQL commands are different between log_statement and
> pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
> log_statement. That's confusing. We should use the same "category-mapping"
> rule as much as possible.

[...]

* Michael Paquier (michael.paquier@gmail.com) wrote:
> And on top of that the following things should be changed:
> - Removal of REGRESS_OPTS which is empty
> - Removal of MODULE, which overlaps with MODULE_big
> - $(WIN32RES) needs to be added to OBJS for Windows versioning

I've pushed these changes.

    Thanks!

        Stephen