On Mon, Mar 5, 2012 at 12:50 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>> The latest version of this patch looks sound to me. We haven't
>>> insisted on having even a sample application for every hook before,
>>> let alone a regression test, so I don't think this patch needs one
>>> either.
>>
>> What we've generally asked for with hooks is a working sample usage of
>> the hook, just as a cross-check that something useful can be done with
>> it and you didn't overlook any obvious usability problems. I agree that
>> a regression test is often not practical, especially not if you're not
>> prepared to create a whole contrib module to provide a sample usage.
>>
>> In the case at hand, ISTM there are some usability questions around
>> where/when the hook is called: in particular, if I'm reading it right,
>> the hook could not override a log_min_messages-based decision that a
>> given message is not to be emitted. Do we care?
>
> That's what I understand too. We could relax that at some stage in the
> future if we had a requirement, I guess.
>
>> Also, if the hook
>> is meant to be able to change the data that gets logged, as seems to be
>> the case, do we care that it would also affect what gets sent to the
>> client?
>>
>> I'd like to see a spec for exactly which fields of ErrorData the hook is
>> allowed to change, and some rationale.
>
>
> Good question. I'd somewhat be inclined to say that it should only be able
> to change output_to_server and output_to_client, and possibly only to change
> them from true to false (i.e. I'm not sure the hook should be able to induce
> more verbose logging.) But maybe that's too restrictive. I doubt we can
> enforce good behaviour, though, only state that if you break things you get
> to keep all the pieces.
It sort of looks like it's intended to apply only to server output, so
I'd find it odd to say that it should be able to mess with
output_to_client.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
