Re: Patch review for logging hooks (CF 2012-01)

On 03/05/2012 12:08 PM, Tom Lane wrote:
> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>> On Sun, Mar 4, 2012 at 10:45 AM, Andrew Dunstan<andrew(at)dunslane(dot)net> wrote:
>>> I'm just looking at this patch, and I agree, it should be testable. I'm
>>> wondering if it wouldn't be a good idea to have a module or set of modules
>>> for demonstrating and testing bits of the API that we expose. src/test/api
>>> or something similar? I'm not sure how we'd automate a test for this case,
>>> though. I guess we could use something like pg_logforward and have a UDP
>>> receiver catch the messages and write them to a file. Something like that
>>> should be possible to rig up in Perl. But all that seems a lot of work at
>>> this stage of the game. So the question is do we want to commit this patch
>>> without it?
>> 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.

cheers

andrew