Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On 12/23/15 9:15 PM, Michael Paquier wrote: On Mon, Dec 7, 2015 at 9:14 PM, Michael Paquierwrote: On Wed, Sep 2, 2015 at 6:19 AM, Marko Tiikkaja wrote: Hopefully nobody minds if I slip this to the commit fest that started today? The attached patch should address all the comments from the 9.5 cycle. All the previous comments have been addressed. Perhaps some regression tests would have some value? This thread has been stalling for a couple of weeks now. I have marked it as "returned with feedback". Marko, if you are still working on this patch, could you add some regression tests and then move it to the next CF? This was submitted to the 2016-03 CF but no new patch was supplied and there's been no activity on the thread for months. I'm marking it as "returned with feedback." Marko, if can address Michael's concerns or supply a new patch I'll be happy to open the CF entry again. Thanks, -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On Mon, Dec 7, 2015 at 9:14 PM, Michael Paquierwrote: > > > On Wed, Sep 2, 2015 at 6:19 AM, Marko Tiikkaja wrote: >> >> Hopefully nobody minds if I slip this to the commit fest that started >> today? The attached patch should address all the comments from the 9.5 >> cycle. > > > All the previous comments have been addressed. Perhaps some regression tests > would have some value? This thread has been stalling for a couple of weeks now. I have marked it as "returned with feedback". Marko, if you are still working on this patch, could you add some regression tests and then move it to the next CF? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On Wed, Sep 2, 2015 at 6:19 AM, Marko Tiikkajawrote: > Hopefully nobody minds if I slip this to the commit fest that started > today? The attached patch should address all the comments from the 9.5 > cycle. > All the previous comments have been addressed. Perhaps some regression tests would have some value? -- Michael
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On 2015-09-01 23:19, I wrote: Hopefully nobody minds if I slip this to the commit fest that started today? The attached patch should address all the comments from the 9.5 cycle. Apparently the CF app will. Meh. Whatever. Ignore then, I guess. .m -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
Hi, Hopefully nobody minds if I slip this to the commit fest that started today? The attached patch should address all the comments from the 9.5 cycle. .m *** a/src/backend/parser/analyze.c --- b/src/backend/parser/analyze.c *** *** 2107,2118 transformUpdateTargetList(ParseState *pstate, List *origTlist) attrno = attnameAttNum(pstate->p_target_relation, origTarget->name, true); if (attrno == InvalidAttrNumber) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("column \"%s\" of relation \"%s\" does not exist", origTarget->name, ! RelationGetRelationName(pstate->p_target_relation)), parser_errposition(pstate, origTarget->location))); updateTargetListEntry(pstate, tle, origTarget->name, attrno, --- 2107,2136 attrno = attnameAttNum(pstate->p_target_relation, origTarget->name, true); if (attrno == InvalidAttrNumber) + { + bool need_hint = false; + + /* +* Table-qualifying the LHS expression in SET is a common mistake; +* provide a hint if that seems to be the problem. +*/ + if (origTarget->indirection != NIL && + IsA(linitial(origTarget->indirection), String)) + { + if (strcmp(origTarget->name, RelationGetRelationName(pstate->p_target_relation)) == 0) + need_hint = true; + else if (target_rte->alias && strcmp(origTarget->name, target_rte->alias->aliasname) == 0) + need_hint = true; + } + ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("column \"%s\" of relation \"%s\" does not exist", origTarget->name, ! RelationGetRelationName(pstate->p_target_relation)), !need_hint ? errhint("Do not qualify an UPDATE target column with the name of the table.") : 0, parser_errposition(pstate, origTarget->location))); + } updateTargetListEntry(pstate, tle, origTarget->name, attrno, -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On Thu, Jan 8, 2015 at 2:28 AM, Marko Tiikkaja ma...@joh.to wrote: Yeah. (The CF entry is also set to Waiting on Author, which seems appropriate.) Seeing nothing happening here for quite some time, marked as returned with feedback.. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
We're waiting for an updated version here, right? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On 1/7/15 6:22 PM, Alvaro Herrera wrote: We're waiting for an updated version here, right? Yeah. (The CF entry is also set to Waiting on Author, which seems appropriate.) .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On 2014-11-22 05:11, Peter Geoghegan wrote: On Fri, Nov 21, 2014 at 7:49 PM, Marko Tiikkaja ma...@joh.to wrote: A common mistake is to try and qualify column references on the LHS of SET in UPDATE. I think that this is a good idea, but as written the patch doesn't handle aliases correctly: postgres=# create table foo (val text); CREATE TABLE postgres=# update foo f set val = 'bar' where f.val != 'fd'; UPDATE 0 postgres=# update foo f set f.val = 'bar' where f.val != 'fd'; ERROR: 42703: column f of relation foo does not exist LINE 1: update foo f set f.val = 'bar' where f.val != 'fd'; ^ LOCATION: transformUpdateStmt, analyze.c:2015 Good point! Changed in v2, attached. .marko *** a/src/backend/parser/analyze.c --- b/src/backend/parser/analyze.c *** *** 1989,2000 transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt) attrno = attnameAttNum(pstate-p_target_relation, origTarget-name, true); if (attrno == InvalidAttrNumber) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg(column \%s\ of relation \%s\ does not exist, origTarget-name, ! RelationGetRelationName(pstate-p_target_relation)), parser_errposition(pstate, origTarget-location))); updateTargetListEntry(pstate, tle, origTarget-name, attrno, --- 1989,2020 attrno = attnameAttNum(pstate-p_target_relation, origTarget-name, true); if (attrno == InvalidAttrNumber) + { + const char *targetrefname; + const char *message_hint = NULL; + + /* +* Table-qualifying the LHS expression in SET is a common mistake; +* provide a hint if that seems to be the problem. +*/ + if (target_rte-alias) + targetrefname = target_rte-alias-aliasname; + else + targetrefname = RelationGetRelationName(pstate-p_target_relation); + + if (strcmp(origTarget-name, targetrefname) == 0 + origTarget-indirection != NIL + IsA(linitial(origTarget-indirection), String)) + message_hint = Target column references in UPDATE must not be qualified; + ereport(ERROR, (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg(column \%s\ of relation \%s\ does not exist, origTarget-name, ! RelationGetRelationName(pstate-p_target_relation)), !message_hint ? errhint(%s, message_hint) : 0, parser_errposition(pstate, origTarget-location))); + } updateTargetListEntry(pstate, tle, origTarget-name, attrno, -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
Marko Tiikkaja ma...@joh.to writes: A common mistake is to try and qualify column references on the LHS of SET in UPDATE. The error message can be a bit cryptic at times, too. Perhaps, but this hint is not much of an improvement: HINT: Target column references in UPDATE must not be qualified because target column references *can* be qualified, for example if you have a composite column datatype you can assign directly to one of its fields. (This ambiguity is exactly why we don't simply allow the case.) So I don't think that qualified is a sufficiently precise phrase to be helpful. Possibly something along the lines of HINT: Do not qualify an UPDATE target column with the name of the table. Also, the coding technique used here is poor, because the hint text will not be exposed for translation. The usual pattern is need_hint ? errhint(Message text here.) : 0 Also, as far as Peter's point goes, it would likely make sense to issue this hint if the column basename is *either* the alias name or the underlying table name. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On 2014-11-22 18:02, Tom Lane wrote: Marko Tiikkaja ma...@joh.to writes: A common mistake is to try and qualify column references on the LHS of SET in UPDATE. The error message can be a bit cryptic at times, too. Perhaps, but this hint is not much of an improvement: HINT: Target column references in UPDATE must not be qualified because target column references *can* be qualified, for example if you have a composite column datatype you can assign directly to one of its fields. (This ambiguity is exactly why we don't simply allow the case.) So I don't think that qualified is a sufficiently precise phrase to be helpful. Possibly something along the lines of HINT: Do not qualify an UPDATE target column with the name of the table. Sounds good to me. I didn't expect anyone to like the wording of the hint in the first place ;-) Also, the coding technique used here is poor, because the hint text will not be exposed for translation. The usual pattern is need_hint ? errhint(Message text here.) : 0 Oops. I just copied what the first relevant grep of errhint did, which was from postgres_fdw. But its hints will already have been translated. Will fix. Also, as far as Peter's point goes, it would likely make sense to issue this hint if the column basename is *either* the alias name or the underlying table name. Yeah, I thought about that too, but I thought that might be weird if there's an alias in FROM with the name of the table, e.g: UPDATE foo f1 SET foo.a = 1 FROM foo; But I don't really care too much either way. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HINTing on UPDATE foo SET foo.bar = ..;
On Fri, Nov 21, 2014 at 7:49 PM, Marko Tiikkaja ma...@joh.to wrote: A common mistake is to try and qualify column references on the LHS of SET in UPDATE. I think that this is a good idea, but as written the patch doesn't handle aliases correctly: postgres=# create table foo (val text); CREATE TABLE postgres=# update foo f set val = 'bar' where f.val != 'fd'; UPDATE 0 postgres=# update foo f set f.val = 'bar' where f.val != 'fd'; ERROR: 42703: column f of relation foo does not exist LINE 1: update foo f set f.val = 'bar' where f.val != 'fd'; ^ LOCATION: transformUpdateStmt, analyze.c:2015 -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers