The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Patch applies cleanly on master (b238527664ec6f6c9d00dba4cc2f3dab1c8b8b04),
compiles, and passes both 'make check-world' and 'make installcheck-world'.
The patch includes changes to the expected output of a few tests, and adds new
code comments and changes existing code comments, but I did not notice any new
tests or new documentation to specifically test or explain the behavioral
change this patch is intended to introduce. None of the code comments seem to
adequately explain what an RTE_RESULT is and when it would be used. This
information can be gleaned with some difficulty by reading every file
containing RTE_RESULT, but that seems rather unfriendly.
As an example of where I could use a bit more documentation, see
src/backend/rewrite/rewriteHandler.c circa line 447; I don't know why the
switch statement lacks a case for RTE_RESULT. Why would RTE_VALUES contain
bare expressions but RTE_RESULT would not? Does this mean that
INSERT INTO mytable VALUES ('foo', 'bar');
differs from
SELECT 'foo', 'bar';
in terms of whether 'foo' and 'bar' are bare expressions? Admittedly, I don't
know this code very well, and this might be obvious to others.
