Robert, * Robert Haas (robertmh...@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 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
signature.asc
Description: Digital signature