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

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)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 solution to
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 (trusting event
triggers to cover everything under CREATE SCHEMA). The event trigger
is correctly collecting the GRANT command and pg_audit is outputting a
record based on that (when DDL is enabled), but it's not detecting
that the subcommand is a GRANT. That's certainly not great, but it's
not an insurmountable problem either- correct the early exit logic,
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 only code-level issue
found by the buildfarm- all of the other commits associated with the
buildfarm were due to how the regression tests 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 it here 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 documented accordingly, as
we felt the comment in the core code was correct, but, even so, we
agreed with Fujii that we should probably 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 decide on 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