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
