Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Mar 4, 2012 at 10:45 AM, Andrew Dunstan <andrew@dunslane.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? 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.
regards, tom lane
