Subject: Re: [HACKERS] MERGE SQL Statement for PG11
To: Peter Geoghegan <pg@bowt.ie>, Robert Haas <robertmhaas@gmail.com>
Cc: Pavan Deolasee <pavan.deolasee@gmail.com>,
 Simon Riggs <simon@2ndquadrant.com>, Alvaro Herrera
 <alvherre@alvh.no-ip.org>, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
 Pavan Deolasee <pavan.deolasee@2ndquadrant.com>,
 Stephen Frost <sfrost@snowman.net>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <CANP8+jKitBSrB7oTgT9CY2i1ObfOt36z0XMraQc+Xrz8QB0nXA@mail.gmail.com>
 <CABOikdPjjG+JcdNeegrL7=BtPdJ6yEv--V4hU8KzJTTwX1SNmw@mail.gmail.com>
 <CAH2-Wzmk7RQ-v1DjhfK==s0ejPxpCWFtnwR3J4tW_t7ZvjT_Sw@mail.gmail.com>
 <CABOikdMXQbSWAMNZL9Xodai8Lc0YpqXo_8ABQTBzqbmijEbJ9g@mail.gmail.com>
 <CAH2-WznUpOvdDyKVMdoOa7f+T3HtdAh4gqKOFQak7m-s0Qjc7Q@mail.gmail.com>
 <CA+Tgmob-UYxQRA12ApFBnrb64EnV2eb0=uykByR-2tgbeNj65A@mail.gmail.com>
 <CAH2-WzkBufhEn0tPtUDZ=jcGBnjAspFAN_Ewr_19TcZuJVdZgw@mail.gmail.com>
 <CABOikdNnJcrEbbZ6+9PGP=czFOi7Wz46T0mK8Cr0KHngoWJT6w@mail.gmail.com>
 <CAH2-WzmrhXJPSPP6_34dV_FDiomE4NabWCq1LHfTPmqyW7EcHg@mail.gmail.com>
 <CABOikdPFCcgp7=zoN4M=y0TefW4Q9dPAU+Oy5jN5A+hWYdnvNg@mail.gmail.com>
 <CAH2-Wz=7asCH50WoOmpjXc7fcdc=gHaz+odsbZL+dp4fuKAsow@mail.gmail.com>
 <CABOikdOUoaXt1H885TC_cA1LoErEejqdVDZqG62rQkiZPPyg0Q@mail.gmail.com>
 <CA+TgmoZVSGP0tz+R-JMguARFeQo5JBXngw=--_SAPSNO2CmTSg@mail.gmail.com>
 <CAH2-WzmnQ81DGDdjD=F1zV_2aS+9oTnM489nzsFJSQu+9nOTCg@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <039bd6c2-eff8-3f3c-6896-fe992f069d1e@2ndquadrant.com>
Date: Fri, 9 Mar 2018 00:29:35 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CAH2-WzmnQ81DGDdjD=F1zV_2aS+9oTnM489nzsFJSQu+9nOTCg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 03/08/2018 11:46 PM, Peter Geoghegan wrote:
> On Thu, Mar 8, 2018 at 6:52 AM, Robert Haas <robertmhaas@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