Re: [COMMITTERS] pgsql: Add pg_audit, an auditing extension

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> I note that there are already 11 followup commits fixing bugs in
> pg_audit, including 7 in the first 24 hours.  It's usually not a good
> thing when you can get the regression tests for a piece of
> platform-independent code to pass in less than 8 tries.  I suspect,
> but do not know for certain, that this is just the tip of the iceberg.

Having many commits immediately following a patch going in is certainly
a reasonable reason to be concerned.  What may not have been apparent is
that all but one of those were specifically due to issues with how the
regression tests were being run rather than any issues in the code.

In an attempt to provide clarity and bring an objective view to the
issues which have been identified in the pg_audit code since it was
committed, I've developed the following list, which I kindly ask you
and Noah to review objectivly.  This list includes all issues that I've
identified from the commits done to master, comments made by the
numerous individuals who have taken time to look at the patch and
comment, and those found by the ongoing work of David and I (which has
continued after the commit).  If you both still feel that these are
indicative of being just 'the tip of the iceberg', then I'll revert it.
I do not ask that you provide any further explanation as it's ultimately
a judgement call.

Note that I intentionally filtered out documentation and comment typo
changes (either committed or planned), and the issues which were caused
by how the regression tests were being run, as those are not germane to
the code quality concerns.

I would ask others to comment, but I can, understandably, appreciate
that they would prefer to stay out of a conversion with such poor tone.

I've attempted to order the list by severity, though that's clearly up
for some debate, so take it for what you feel it is worth.

SPI query qualify - Noah
 This is absolutely an issue, but we are kidding ourselves if we believe that this will never happen again.  A holistic
solutionto this issue is needed.
 

CREATE SCHEMA .. GRANT - Noah
 This is definitely an issue which needs to be addressed, but I don't believe this is an issue with the approach used
(trustingevent triggers to cover everything under CREATE SCHEMA).  The event trigger is correctly collecting the GRANT
commandand pg_audit is outputting a record based on that (when DDL is enabled), but it's not detecting that the
subcommandis a GRANT.  That's certainly not great, but it's not an insurmountable problem either- correct the early
exitlogic, pull the command tag from pg_event_trigger_ddl_commands and use it.
 

Portability issue wrt %ld - Tom
 Clearly an issue, but one which the buildfarm is specifically intended to catch and address.  Note that this is the
onlycode-level issue found by the buildfarm- all of the other commits associated with the buildfarm were due to how the
regressiontests were being run.
 

T_AlterDefaultPrivilegesStmt classification fix - Noah
 This should clearly be ROLE instead of DDL.

Use ereport() instead of elog() - Tom
 Clearly better, but I don't see how this could be exploited or necessairly rises to the level of 'bug', but I included
ithere for completeness.
 

Reclassify REINDEX - Fujii
 This was done to match what is done in core regarding REINDEX.  Note that it was intentionally set to DDL, and
documentedaccordingly, as we felt the comment in the core code was correct, but, even so, we agreed with Fujii that we
shouldprobably match what core actually does here regardless.
 

Suppress STATEMENT output - Fujii
 Makes sense but is just about reducing the amount of log traffic.

Suppress CONTEXT output - David
 Similar to the STATEMENT case above, creates unnecessary noise in the logs.

Further classification questions - Fujii
 These questions will undoubtably come up and there isn't one answer here but rather a case where we simply need to
decideon something and document it accordingly.
 

Remove ERROR, PANIC, and FATAL from log_level options - David
 This is a SUSET variable, so there isn't a particular security risk here, but it doesn't make sense to include these
options.

I'm hopeful, as I'm sure you understand, that the number of issues being
reported is due principally to the amount of review and testing which
has happened on this module since it went in and not because the quality
of the code is particularly poor.  I feel it's far more than happens for
many commits, even ones which are backpatched, and I'm certainly hopeful
that it makes for a better end result.  I certainly do not intend to ask
the community to accept a patch which is bugridden or full of holes, nor
will I stand in the way of the community requesting that a feature be
reverted.
Thanks!
    Stephen