Re: [HACKERS] Transaction control in procedures

On 12/06/2017 09:41 AM, Peter Eisentraut wrote:
> On 12/5/17 13:33, Robert Haas wrote:
>> On Tue, Dec 5, 2017 at 1:25 PM, Peter Eisentraut
>> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>>> I think ROLLBACK in a cursor loop might not make sense, because the
>>> cursor query itself could have side effects, so a rollback would have to
>>> roll back the entire loop. That might need more refined analysis before
>>> it could be allowed.
>> COMMIT really has the same problem; if the cursor query has side
>> effects, you can't commit those side effects piecemeal as the loop
>> executed and have things behave sanely.
> The first COMMIT inside the loop would commit the cursor query. This
> isn't all that different from what you'd get now if you coded this
> manually using holdable cursors or just plain client code. Clearly, you
> can create a mess if the loop body interacts with the loop expression,
> but that's already the case.
>
> But if you coded something like this yourself now and ran a ROLLBACK
> inside the loop, the holdable cursor would disappear (unless previously
> committed), so you couldn't proceed with the loop.
>
> The SQL standard for persistent stored modules explicitly prohibits
> COMMIT and ROLLBACK in cursor loop bodies. But I think people will
> eventually want it.
>
>>>> - COMMIT or ROLLBACK inside a procedure with a SET clause attached,
>>> That also needs to be prohibited because of the way the GUC nesting
>>> currently works. It's probably possible to fix it, but it would be a
>>> separate effort.
>>>
>>>> and/or while SET LOCAL is in effect either at the inner or outer
>>>> level.
>>> That seems to work fine.
>> These two are related -- if you don't permit anything that makes
>> temporary changes to GUCs at all, like SET clauses attached to
>> functions, then SET LOCAL won't cause any problems. The problem is if
>> you do a transaction operation when something set locally is in the
>> stack of values, but not at the top.
> Yes, that's exactly the problem. So right now I'm just preventing the
> problematic scenario. So fix that, one would possibly have to replace
> the stack by something not quite a stack.
>
>
> New patch attached.
>

In general this looks good. However, I'm getting runtime errors in
plperl_elog.c on two different Linux platforms (Fedora 25, Ubuntu
16.04). There seems to be something funky going on. And we do need to
work out why the commented out plperl test isn't working.

Detailed comments:

Referring to anonymous blocks might be a bit mystifying for some
readers, unless we note that they are invoked via DO.

I think this sentence should probably be worded a bit differently:

(And of course, BEGIN and END have different meanings in PL/pgSQL.)

Perhaps "Note that" instead of "And of course,".

Why aren't the SPI functions that error out or return 0 just void
instead of returning an int?

The docs say SPI_set_non_atomic() returns 0 on success, but the code
says otherwise.

This sentence in the comment in SPI_Commit() is slightly odd:

This restriction is particular to PLs implemented on top of SPI.

Perhaps "required by" rather than "particular to" would make it read better.

The empty free_commit() and free_rollback() functions in pl_funcs.c look
slightly odd. I realize that the compiler should optimize the calls
away, but it seems an odd style.

One other thing I wondered about was what if a PL function (say plperl)
used SPI to open an explicit cursor and then looped over it? If there
were a commit or rollback inside that loop we wouldn't have the same
protections we have in plpgsql, ISTM. I haven't tried this yet, so I'm
just speculating about what might happen.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services