On 03/08/2018 11:46 PM, Peter Geoghegan wrote: > On Thu, Mar 8, 2018 at 6:52 AM, Robert Haas <robertmh...@gmail.com> wrote: >>> I removed this code since it was wrong. We might want to add some basic >>> checks for existence of volatile functions in the WHEN or SET clauses. But I >>> agree, it's no different than regular UPDATEs. So may be not a big deal. >> >> I just caught up on this thread. I'm definitely glad to see that code >> go because, wow, that is all kinds of wrong. I don't see a real need >> to add any kind of replacement check, either. Prohibiting volatile >> functions here doesn't seem likely to accomplish anything useful. It >> seems like the most we'd want to do is mention this the documentation >> somehow, and I'm not even sure we really need to do that much. > > Thanks in large part to Pavan's excellent work, the situation in > nodeModifyTable.c is much clearer than it was a few weeks ago. It's > now obvious that MERGE is very similar to UPDATE ... FROM, which > doesn't have any restrictions on volatile functions. >
Yeah, I agree Pavan did an excellent work on moving this patch forward. > I don't see any sense in prohibiting volatile functions in either > case, because it should be obvious to users that that's just asking > for trouble. I can believe that someone would make that mistake, just > about, but they'd have to be writing their DML statement on > auto-pilot. > The reason why the patch tried to prevent that is because the SQL standard says this (p. 1176 of SQL 2016): 15) The <search condition> immediately contained in a <merge statement>, the <search condition> immediately contained in a <merge when matched clause>, and the <search condition> immediately contained in a <merge when not matched clause> shall not generally contain a <routine invocation> whose subject routine is an SQL-invoked routine that possibly modifies SQL-data. I'm not quite sure what is required to be compliant with this rule. For example what does "immediately contained" or "shall not generally contain" mean? Does that mean user are expected not to do that because it's obviously silly, or do we need to implement some protection? That being said the volatility check seems reasonable to me (and i would not expect it to be a huge amount of code). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
