Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 056a5a3

Browse files
committed
Allow committing inside cursor loop
Previously, committing or aborting inside a cursor loop was prohibited because that would close and remove the cursor. To allow that, automatically convert such cursors to holdable cursors so they survive commits or rollbacks. Portals now have a new state "auto-held", which means they have been converted automatically from pinned. An auto-held portal is kept on transaction commit or rollback, but is still removed when returning to the main loop on error. This supports all languages that have cursor loop constructs: PL/pgSQL, PL/Python, PL/Perl. Reviewed-by: Ildus Kurbangaliev <i.kurbangaliev@postgrespro.ru>
1 parent a2894cc commit 056a5a3

File tree

15 files changed

+534
-87
lines changed

15 files changed

+534
-87
lines changed

doc/src/sgml/plperl.sgml

-5
Original file line numberDiff line numberDiff line change
@@ -722,11 +722,6 @@ $$;
722722
CALL transaction_test1();
723723
</programlisting>
724724
</para>
725-
726-
<para>
727-
Transactions cannot be ended when a cursor created by
728-
<function>spi_query</function> is open.
729-
</para>
730725
</listitem>
731726
</varlistentry>
732727
</variablelist>

doc/src/sgml/plpgsql.sgml

+35-2
Original file line numberDiff line numberDiff line change
@@ -3510,8 +3510,41 @@ CALL transaction_test1();
35103510
</para>
35113511

35123512
<para>
3513-
A transaction cannot be ended inside a loop over a query result, nor
3514-
inside a block with exception handlers.
3513+
Special considerations apply to cursor loops. Consider this example:
3514+
<programlisting>
3515+
CREATE PROCEDURE transaction_test2()
3516+
LANGUAGE plpgsql
3517+
AS $$
3518+
DECLARE
3519+
r RECORD;
3520+
BEGIN
3521+
FOR r IN SELECT * FROM test2 ORDER BY x LOOP
3522+
INSERT INTO test1 (a) VALUES (r.x);
3523+
COMMIT;
3524+
END LOOP;
3525+
END;
3526+
$$;
3527+
3528+
CALL transaction_test2();
3529+
</programlisting>
3530+
Normally, cursors are automatically closed at transaction commit.
3531+
However, a cursor created as part of a loop like this is automatically
3532+
converted to a holdable cursor by the first <command>COMMIT</command> or
3533+
<command>ROLLBACK</command>. That means that the cursor is fully
3534+
evaluated at the first <command>COMMIT</command> or
3535+
<command>ROLLBACK</command> rather than row by row. The cursor is still
3536+
removed automatically after the loop, so this is mostly invisible to the
3537+
user.
3538+
</para>
3539+
3540+
<para>
3541+
Transaction commands are not allowed in cursor loops driven by commands
3542+
that are not read-only (for example <command>UPDATE
3543+
... RETURNING</command>).
3544+
</para>
3545+
3546+
<para>
3547+
A transaction cannot be ended inside a block with exception handlers.
35153548
</para>
35163549
</sect1>
35173550

doc/src/sgml/plpython.sgml

+1-3
Original file line numberDiff line numberDiff line change
@@ -1416,9 +1416,7 @@ CALL transaction_test1();
14161416
</para>
14171417

14181418
<para>
1419-
Transactions cannot be ended when a cursor created by
1420-
<literal>plpy.cursor</literal> is open or when an explicit subtransaction
1421-
is active.
1419+
Transactions cannot be ended when an explicit subtransaction is active.
14221420
</para>
14231421
</sect1>
14241422

src/backend/tcop/postgres.c

+2
Original file line numberDiff line numberDiff line change
@@ -3938,6 +3938,8 @@ PostgresMain(int argc, char *argv[],
39383938
if (am_walsender)
39393939
WalSndErrorCleanup();
39403940

3941+
PortalErrorCleanup();
3942+
39413943
/*
39423944
* We can't release replication slots inside AbortTransaction() as we
39433945
* need to be able to start and abort transactions while having a slot

src/backend/utils/mmgr/portalmem.c

+105-33
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,36 @@ PortalHashTableDeleteAll(void)
620620
}
621621
}
622622

623+
/*
624+
* "Hold" a portal. Prepare it for access by later transactions.
625+
*/
626+
static void
627+
HoldPortal(Portal portal)
628+
{
629+
/*
630+
* Note that PersistHoldablePortal() must release all resources
631+
* used by the portal that are local to the creating transaction.
632+
*/
633+
PortalCreateHoldStore(portal);
634+
PersistHoldablePortal(portal);
635+
636+
/* drop cached plan reference, if any */
637+
PortalReleaseCachedPlan(portal);
638+
639+
/*
640+
* Any resources belonging to the portal will be released in the
641+
* upcoming transaction-wide cleanup; the portal will no longer
642+
* have its own resources.
643+
*/
644+
portal->resowner = NULL;
645+
646+
/*
647+
* Having successfully exported the holdable cursor, mark it as
648+
* not belonging to this transaction.
649+
*/
650+
portal->createSubid = InvalidSubTransactionId;
651+
portal->activeSubid = InvalidSubTransactionId;
652+
}
623653

624654
/*
625655
* Pre-commit processing for portals.
@@ -648,9 +678,10 @@ PreCommit_Portals(bool isPrepare)
648678

649679
/*
650680
* There should be no pinned portals anymore. Complain if someone
651-
* leaked one.
681+
* leaked one. Auto-held portals are allowed; we assume that whoever
682+
* pinned them is managing them.
652683
*/
653-
if (portal->portalPinned)
684+
if (portal->portalPinned && !portal->autoHeld)
654685
elog(ERROR, "cannot commit while a portal is pinned");
655686

656687
/*
@@ -684,29 +715,7 @@ PreCommit_Portals(bool isPrepare)
684715
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
685716
errmsg("cannot PREPARE a transaction that has created a cursor WITH HOLD")));
686717

687-
/*
688-
* Note that PersistHoldablePortal() must release all resources
689-
* used by the portal that are local to the creating transaction.
690-
*/
691-
PortalCreateHoldStore(portal);
692-
PersistHoldablePortal(portal);
693-
694-
/* drop cached plan reference, if any */
695-
PortalReleaseCachedPlan(portal);
696-
697-
/*
698-
* Any resources belonging to the portal will be released in the
699-
* upcoming transaction-wide cleanup; the portal will no longer
700-
* have its own resources.
701-
*/
702-
portal->resowner = NULL;
703-
704-
/*
705-
* Having successfully exported the holdable cursor, mark it as
706-
* not belonging to this transaction.
707-
*/
708-
portal->createSubid = InvalidSubTransactionId;
709-
portal->activeSubid = InvalidSubTransactionId;
718+
HoldPortal(portal);
710719

711720
/* Report we changed state */
712721
result = true;
@@ -771,6 +780,14 @@ AtAbort_Portals(void)
771780
if (portal->createSubid == InvalidSubTransactionId)
772781
continue;
773782

783+
/*
784+
* Do nothing to auto-held cursors. This is similar to the case of a
785+
* cursor from a previous transaction, but it could also be that the
786+
* cursor was auto-held in this transaction, so it wants to live on.
787+
*/
788+
if (portal->autoHeld)
789+
continue;
790+
774791
/*
775792
* If it was created in the current transaction, we can't do normal
776793
* shutdown on a READY portal either; it might refer to objects
@@ -834,8 +851,11 @@ AtCleanup_Portals(void)
834851
if (portal->status == PORTAL_ACTIVE)
835852
continue;
836853

837-
/* Do nothing to cursors held over from a previous transaction */
838-
if (portal->createSubid == InvalidSubTransactionId)
854+
/*
855+
* Do nothing to cursors held over from a previous transaction or
856+
* auto-held ones.
857+
*/
858+
if (portal->createSubid == InvalidSubTransactionId || portal->autoHeld)
839859
{
840860
Assert(portal->status != PORTAL_ACTIVE);
841861
Assert(portal->resowner == NULL);
@@ -865,6 +885,32 @@ AtCleanup_Portals(void)
865885
}
866886
}
867887

888+
/*
889+
* Portal-related cleanup when we return to the main loop on error.
890+
*
891+
* This is different from the cleanup at transaction abort. Auto-held portals
892+
* are cleaned up on error but not on transaction abort.
893+
*/
894+
void
895+
PortalErrorCleanup(void)
896+
{
897+
HASH_SEQ_STATUS status;
898+
PortalHashEnt *hentry;
899+
900+
hash_seq_init(&status, PortalHashTable);
901+
902+
while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL)
903+
{
904+
Portal portal = hentry->portal;
905+
906+
if (portal->autoHeld)
907+
{
908+
portal->portalPinned = false;
909+
PortalDrop(portal, false);
910+
}
911+
}
912+
}
913+
868914
/*
869915
* Pre-subcommit processing for portals.
870916
*
@@ -1164,8 +1210,19 @@ ThereAreNoReadyPortals(void)
11641210
return true;
11651211
}
11661212

1167-
bool
1168-
ThereArePinnedPortals(void)
1213+
/*
1214+
* Hold all pinned portals.
1215+
*
1216+
* A procedural language implementation that uses pinned portals for its
1217+
* internally generated cursors can call this in its COMMIT command to convert
1218+
* those cursors to held cursors, so that they survive the transaction end.
1219+
* We mark those portals as "auto-held" so that exception exit knows to clean
1220+
* them up. (In normal, non-exception code paths, the PL needs to clean those
1221+
* portals itself, since transaction end won't do it anymore, but that should
1222+
* be normal practice anyway.)
1223+
*/
1224+
void
1225+
HoldPinnedPortals(void)
11691226
{
11701227
HASH_SEQ_STATUS status;
11711228
PortalHashEnt *hentry;
@@ -1176,9 +1233,24 @@ ThereArePinnedPortals(void)
11761233
{
11771234
Portal portal = hentry->portal;
11781235

1179-
if (portal->portalPinned)
1180-
return true;
1181-
}
1236+
if (portal->portalPinned && !portal->autoHeld)
1237+
{
1238+
/*
1239+
* Doing transaction control, especially abort, inside a cursor
1240+
* loop that is not read-only, for example using UPDATE
1241+
* ... RETURNING, has weird semantics issues. Also, this
1242+
* implementation wouldn't work, because such portals cannot be
1243+
* held. (The core grammar enforces that only SELECT statements
1244+
* can drive a cursor, but for example PL/pgSQL does not restrict
1245+
* it.)
1246+
*/
1247+
if (portal->strategy != PORTAL_ONE_SELECT)
1248+
ereport(ERROR,
1249+
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
1250+
errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));
11821251

1183-
return false;
1252+
portal->autoHeld = true;
1253+
HoldPortal(portal);
1254+
}
1255+
}
11841256
}

src/include/utils/portal.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,8 @@ typedef struct PortalData
147147
/* Status data */
148148
PortalStatus status; /* see above */
149149
bool portalPinned; /* a pinned portal can't be dropped */
150+
bool autoHeld; /* was automatically converted from pinned to
151+
* held (see HoldPinnedPortals()) */
150152

151153
/* If not NULL, Executor is active; call ExecutorEnd eventually: */
152154
QueryDesc *queryDesc; /* info needed for executor invocation */
@@ -204,6 +206,7 @@ extern void EnablePortalManager(void);
204206
extern bool PreCommit_Portals(bool isPrepare);
205207
extern void AtAbort_Portals(void);
206208
extern void AtCleanup_Portals(void);
209+
extern void PortalErrorCleanup(void);
207210
extern void AtSubCommit_Portals(SubTransactionId mySubid,
208211
SubTransactionId parentSubid,
209212
ResourceOwner parentXactOwner);
@@ -231,6 +234,6 @@ extern PlannedStmt *PortalGetPrimaryStmt(Portal portal);
231234
extern void PortalCreateHoldStore(Portal portal);
232235
extern void PortalHashTableDeleteAll(void);
233236
extern bool ThereAreNoReadyPortals(void);
234-
extern bool ThereArePinnedPortals(void);
237+
extern void HoldPinnedPortals(void);
235238

236239
#endif /* PORTAL_H */

src/pl/plperl/expected/plperl_transaction.out

+67-4
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,44 @@ while (defined($row = spi_fetchrow($sth))) {
105105
spi_commit();
106106
}
107107
$$;
108-
ERROR: cannot commit transaction while a cursor is open at line 6.
109-
CONTEXT: PL/Perl anonymous code block
110108
SELECT * FROM test1;
111109
a | b
112110
---+---
111+
0 |
112+
1 |
113+
2 |
114+
3 |
115+
4 |
116+
(5 rows)
117+
118+
-- check that this doesn't leak a holdable portal
119+
SELECT * FROM pg_cursors;
120+
name | statement | is_holdable | is_binary | is_scrollable | creation_time
121+
------+-----------+-------------+-----------+---------------+---------------
122+
(0 rows)
123+
124+
-- error in cursor loop with commit
125+
TRUNCATE test1;
126+
DO LANGUAGE plperl $$
127+
my $sth = spi_query("SELECT * FROM test2 ORDER BY x");
128+
my $row;
129+
while (defined($row = spi_fetchrow($sth))) {
130+
spi_exec_query("INSERT INTO test1 (a) VALUES (12/(" . $row->{x} . "-2))");
131+
spi_commit();
132+
}
133+
$$;
134+
ERROR: division by zero at line 5.
135+
CONTEXT: PL/Perl anonymous code block
136+
SELECT * FROM test1;
137+
a | b
138+
-----+---
139+
-6 |
140+
-12 |
141+
(2 rows)
142+
143+
SELECT * FROM pg_cursors;
144+
name | statement | is_holdable | is_binary | is_scrollable | creation_time
145+
------+-----------+-------------+-----------+---------------+---------------
113146
(0 rows)
114147

115148
-- rollback inside cursor loop
@@ -122,12 +155,42 @@ while (defined($row = spi_fetchrow($sth))) {
122155
spi_rollback();
123156
}
124157
$$;
125-
ERROR: cannot abort transaction while a cursor is open at line 6.
126-
CONTEXT: PL/Perl anonymous code block
127158
SELECT * FROM test1;
128159
a | b
129160
---+---
130161
(0 rows)
131162

163+
SELECT * FROM pg_cursors;
164+
name | statement | is_holdable | is_binary | is_scrollable | creation_time
165+
------+-----------+-------------+-----------+---------------+---------------
166+
(0 rows)
167+
168+
-- first commit then rollback inside cursor loop
169+
TRUNCATE test1;
170+
DO LANGUAGE plperl $$
171+
my $sth = spi_query("SELECT * FROM test2 ORDER BY x");
172+
my $row;
173+
while (defined($row = spi_fetchrow($sth))) {
174+
spi_exec_query("INSERT INTO test1 (a) VALUES (" . $row->{x} . ")");
175+
if ($row->{x} % 2 == 0) {
176+
spi_commit();
177+
} else {
178+
spi_rollback();
179+
}
180+
}
181+
$$;
182+
SELECT * FROM test1;
183+
a | b
184+
---+---
185+
0 |
186+
2 |
187+
4 |
188+
(3 rows)
189+
190+
SELECT * FROM pg_cursors;
191+
name | statement | is_holdable | is_binary | is_scrollable | creation_time
192+
------+-----------+-------------+-----------+---------------+---------------
193+
(0 rows)
194+
132195
DROP TABLE test1;
133196
DROP TABLE test2;

0 commit comments

Comments
 (0)