Thread: committing inside cursor loop
Here is a patch that allows COMMIT inside cursor loops in PL/pgSQL. As alluded to in earlier threads, this is done by converting such cursors to holdable automatically. A special flag "auto-held" marks such cursors, so we know to clean them up on exceptions. This is currently only for PL/pgSQL, but the same technique could be applied easily to other PLs. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Here is a patch that allows COMMIT inside cursor loops in PL/pgSQL. As
> alluded to in earlier threads, this is done by converting such cursors
> to holdable automatically. A special flag "auto-held" marks such
> cursors, so we know to clean them up on exceptions.
I haven't really read this patch, but this bit jumped out at me:
+ Inside a cursor loop, <command>ROLLBACK</command> is not allowed. (That
+ would have to roll back the cursor query, thus invalidating the loop.)
Say what? That seems to translate into "we have lost the ability to
deal with errors". I don't think this is really what people are hoping
to get out of the transactional-procedure facility.
regards, tom lane
On 20 February 2018 at 14:11, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Here is a patch that allows COMMIT inside cursor loops in PL/pgSQL. As > alluded to in earlier threads, this is done by converting such cursors > to holdable automatically. A special flag "auto-held" marks such > cursors, so we know to clean them up on exceptions. > > This is currently only for PL/pgSQL, but the same technique could be > applied easily to other PLs. Amazingly clean, looks great. I notice that PersistHoldablePortal() does ExecutorRewind(). In most cases, the cursor loop doesn't ever rewind. So it would be good if we could pass a parameter that skips the rewind since it will never be needed and causes a performance hit. What I imagine is we can just persist the as-yet unfetched portion of the cursor from the current row onwards, rather than rewind and store the whole cursor result. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 20 February 2018 at 14:45, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> Here is a patch that allows COMMIT inside cursor loops in PL/pgSQL. As >> alluded to in earlier threads, this is done by converting such cursors >> to holdable automatically. A special flag "auto-held" marks such >> cursors, so we know to clean them up on exceptions. > > I haven't really read this patch, but this bit jumped out at me: > > + Inside a cursor loop, <command>ROLLBACK</command> is not allowed. (That > + would have to roll back the cursor query, thus invalidating the loop.) Hmm, why? Rollback would only invalidate the cursor if it hadn't yet hit a Commit. But if you did a commit, then the cursor would become holdable and you could happily continue reading through the loop even after the rollback. So if Commit changes a pinned portal into a holdable cursor, we just make Rollback do that also. Obviously only for pinned portals, i.e. the query/ies whose results we are currently looping through. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, 20 Feb 2018 09:11:50 -0500 Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Here is a patch that allows COMMIT inside cursor loops in PL/pgSQL. > As alluded to in earlier threads, this is done by converting such > cursors to holdable automatically. A special flag "auto-held" marks > such cursors, so we know to clean them up on exceptions. > > This is currently only for PL/pgSQL, but the same technique could be > applied easily to other PLs. > Hi, The patch still applies, tests passed. The code looks nice, documentation and tests are there. Although as was discussed before it seems inconsistent without ROLLBACK support. There was a little discussion about it, but no replies. Maybe the status of the patch should be changed to 'Waiting on author' until the end of discussion. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
On 3/6/18 07:48, Ildus Kurbangaliev wrote:
> Although as was discussed before it seems inconsistent without ROLLBACK
> support. There was a little discussion about it, but no replies. Maybe
> the status of the patch should be changed to 'Waiting on author' until
> the end of discussion.
I'm wondering what the semantics of it should be.
For example, consider this:
drop table test1;
create table test1 (a int, b int);
insert into test1 values (1, 11), (2, 22), (3, 33);
do
language plpgsql
$$
declare
x int;
begin
for x in update test1 set b = b + 1 where b > 20 returning a loop
raise info 'x = %', x;
if x = 2 then
rollback;
end if;
end loop;
end;
$$;
The ROLLBACK call in the first loop iteration undoes the UPDATE command
that drives the loop. Is it then sensible to continue the loop?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, 13 Mar 2018 11:08:36 -0400 Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/6/18 07:48, Ildus Kurbangaliev wrote: > > Although as was discussed before it seems inconsistent without > > ROLLBACK support. There was a little discussion about it, but no > > replies. Maybe the status of the patch should be changed to > > 'Waiting on author' until the end of discussion. > > I'm wondering what the semantics of it should be. > > For example, consider this: > > drop table test1; > create table test1 (a int, b int); > insert into test1 values (1, 11), (2, 22), (3, 33); > > do > language plpgsql > $$ > declare > x int; > begin > for x in update test1 set b = b + 1 where b > 20 returning a loop > raise info 'x = %', x; > if x = 2 then > rollback; > end if; > end loop; > end; > $$; > > The ROLLBACK call in the first loop iteration undoes the UPDATE > command that drives the loop. Is it then sensible to continue the > loop? > I think that in the first place ROLLBACK was prohibited because of cases like this, but it seems to safe to continue the loop when portal strategy is PORTAL_ONE_SELECT. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
On 3/14/18 08:05, Ildus Kurbangaliev wrote: >> The ROLLBACK call in the first loop iteration undoes the UPDATE >> command that drives the loop. Is it then sensible to continue the >> loop? >> > I think that in the first place ROLLBACK was prohibited because of cases > like this, but it seems to safe to continue the loop when portal > strategy is PORTAL_ONE_SELECT. Maybe, but even plain SELECT commands can have side effects. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/19/18 20:40, Peter Eisentraut wrote: > On 3/14/18 08:05, Ildus Kurbangaliev wrote: >>> The ROLLBACK call in the first loop iteration undoes the UPDATE >>> command that drives the loop. Is it then sensible to continue the >>> loop? >>> >> I think that in the first place ROLLBACK was prohibited because of cases >> like this, but it seems to safe to continue the loop when portal >> strategy is PORTAL_ONE_SELECT. > > Maybe, but even plain SELECT commands can have side effects. Here is an updated patch that supports the ROLLBACK case as well, and prevents holding portals with a strategy other than PORTAL_ONE_SELECT. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed I have checked new version. Although I can miss something, the patch looks good to me. The new status of this patch is: Ready for Committer
On 3/28/18 11:34, Ildus Kurbangaliev wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: tested, passed > > I have checked new version. Although I can miss something, the patch looks good to me. > > The new status of this patch is: Ready for Committer Committed, thanks! -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: Peter> Committed, thanks! So... what is a pl/* that does _not_ use pinned cursors for cursor loops supposed to do in this case? -- Andrew (irc:RhodiumToad)
On 3/29/18 07:37, Andrew Gierth wrote: >>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > > Peter> Committed, thanks! > > So... what is a pl/* that does _not_ use pinned cursors for cursor loops > supposed to do in this case? Other than maybe using pinned cursors, one would have to look at the whole picture and see what makes sense. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> So... what is a pl/* that does _not_ use pinned cursors for cursor >> loops supposed to do in this case? Peter> Other than maybe using pinned cursors, one would have to look at Peter> the whole picture and see what makes sense. Never mind, I was overlooking the obvious: explicitly specifying CURSOR_OPT_HOLD is sufficient for me. (Can't really use pinned cursors because they might not be unpinned fast enough due to timing of garbage collection, which would lead to spurious errors.) -- Andrew (irc:RhodiumToad)