The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
This patch is straightforward, does what it says, and passes the tests.
Regarding the duplication of code between plsample_func_handler and
plsample_trigger_handler, perhaps that's for the best for now, as 3554 in
the same commitfest also touches plsample, so merge conflicts may be
minimized by not doing more invasive refactoring.
That would leave low-hanging fruit for a later patch that could refactor
plsample to reduce the duplication (maybe adding a validator at the same
time? That would also duplicate some of the checks in the existing handlers.)
I am not sure that structuring the trigger handler with separate compile and
execute steps is worth the effort for a simple example like plsample. The main
plsample_func_handler is not so structured.
It's likely that many real PLs will have some notion of compilation separate from
execution. But those will also have logic to do the compilation only once, and
somewhere to cache the result of that for reuse across calls, and those kinds of
details might make plsample's basic skeleton more complex than needed.
I know that in just looking at expected/plsample.out, I was a little distracted by
seeing multiple "compile" messages for the same trigger function in the same
session and wondering why that was.
So maybe it would be simpler and less distracting to assume that the PL targeted
by plsample is one that just has a simple interpreter that works from the source text.
Regards,
-Chap
The new status of this patch is: Waiting on Author
