committing inside cursor loop

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

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)