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
thispatch is intended to introduce. None of the code comments seem to adequately explain what an RTE_RESULT is and
whenit 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'tknow why the switch statement lacks a case for RTE_RESULT. Why would RTE_VALUES contain bare expressions but
RTE_RESULTwould 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
beobvious to others.
