From: | igor polishchuk <ora4dba(at)gmail(dot)com> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Cc: | marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi |
Subject: | INSERT and parentheses |
Date: | 2010-08-24 05:25:41 |
Message-ID: | AANLkTin7oyhDXg89ca=1WKXMke7G9O0Xf5kseXbS6qGX@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)4070605(at)cs(dot)helsinki(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Joe Conway | 2010-08-24 06:24:41 | Re: Typing Records |
Previous Message | KaiGai Kohei | 2010-08-24 05:19:12 | Re: security hook on authorization |