Marko et al,
This is my first ever attempt of a patch review just for learning the
procedure. I'm not a postgres developer, so the review is partial and mostly
from the usability prospective.
The original message id of the patch is
4bd58db3.4070...@cs.helsinki.fi<http://archives.postgresql.org/pgsql-hackers/2010-04/msg01200.php>
Submssion review
=========================
* Is he patch in context diff format?
Ys
* Dos it apply cleanly to the current CVS HEAD?
Applies cleanly to a source code snapshot
* Dos it include reasonable tests, necessary doc patches, etc?
I does not require a doc patch. A test is not included, but it looks pretty
trivial:
-- Prpare the test tables
drop table if exists foo;
drop table if exists boo;
crate table foo(
a nt,
b nt,
c nt);
crate table boo(
a nt,
b nt,
c nt);
insert into boo values (10,20,30);
-- Actual test
INSERT INTO foo(a,b,c) SELECT (a,b,c) FROM boo;
INSERT INTO foo(a,b,c) VALUES((0,1,2));
Usability Review
=========================
The patch provides a HINT for unclear error. This should clarify for a user
what exactly is wrong with the sql.
However, the actual HINT text provided with the patch is not very clear,
too.
The Stephen Frost's suggestion would add clarity:
errhint("insert appears to be a single column with a record-type rather than
multiple columns of non-composite type."),
Feature test
=========================
The feature works as advertised for the test provided above.
No failures or crashes.
No visible affect on performance
