|From:||Mark Dilger <hornschnorter(at)gmail(dot)com>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|Subject:||Re: "SELECT ... FROM DUAL" is not quite as silly as it appears|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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');
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.
|Next Message||Mark Dilger||2018-11-28 16:55:21||Re: "SELECT ... FROM DUAL" is not quite as silly as it appears|
|Previous Message||Evgeniy Efimkin||2018-11-28 16:41:45||Re: [WIP] CREATE SUBSCRIPTION with FOR TABLES clause (table filter)|