database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-07-17 06:41:09 +03:00
Code Activity

Fix TOAST access failure in RETURNING queries.

Browse Source 
Discussion of commit 3e2f3c2e4 exposed a problem that is of longer
standing: since we don't detoast data while sticking it into a portal's
holdStore for PORTAL_ONE_RETURNING and PORTAL_UTIL_SELECT queries, and we
release the query's snapshot as soon as we're done loading the holdStore,
later readout of the holdStore can do TOAST fetches against data that can
no longer be seen by any of the session's live snapshots.  This means that
a concurrent VACUUM could remove the TOAST data before we can fetch it.
Commit 3e2f3c2e4 exposed the problem by showing that sometimes we had *no*
live snapshots while fetching TOAST data, but we'd be at risk anyway.

I believe this code was all right when written, because our management of a
session's exposed xmin was such that the TOAST references were safe until
end of transaction.  But that's no longer true now that we can advance or
clear our PGXACT.xmin intra-transaction.

To fix, copy the query's snapshot during FillPortalStore() and save it in
the Portal; release it only when the portal is dropped.  This essentially
implements a policy that we must hold a relevant snapshot whenever we
access potentially-toasted data.  We had already come to that conclusion
in other places, cf commits 08e261cbc9 and ec543db77b.

I'd have liked to add a regression test case for this, but I didn't see
a way to make one that's not unreasonably bloated; it seems to require
returning a toasted value to the client, and those will be big.

In passing, improve PortalRunUtility() so that it positively verifies
that its ending PopActiveSnapshot() call will pop the expected snapshot,
removing a rather shaky assumption about which utility commands might
do their own PopActiveSnapshot().  There's no known bug here, but now
that we're actively referencing the snapshot it's almost free to make
this code a bit more bulletproof.

We might want to consider back-patching something like this into older
branches, but it would be prudent to let it prove itself more in HEAD
beforehand.

Discussion: <87vazemeda.fsf@credativ.de>
This commit is contained in:
Tom Lane
2016-08-07 17:46:08 -04:00
parent 07a601eeda
commit 9ee1cf04ab
4 changed files with 80 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
src/backend/commands/portalcmds.c
View File

@ -317,10 +317,11 @@ PersistHoldablePortal(Portal portal)
Assert(queryDesc != NULL);
/*
* Caller must have created the tuplestore already.
* Caller must have created the tuplestore already ... but not a snapshot.
*/
Assert(portal->holdContext != NULL);
Assert(portal->holdStore != NULL);
Assert(portal->holdSnapshot == NULL);
/*
* Before closing down the executor, we must copy the tupdesc into
@ -362,7 +363,8 @@ PersistHoldablePortal(Portal portal)
/*
* Change the destination to output to the tuplestore. Note we tell
* the tuplestore receiver to detoast all data passed through it.
* the tuplestore receiver to detoast all data passed through it; this
* makes it safe to not keep a snapshot associated with the data.
*/
queryDesc->dest = CreateDestReceiver(DestTuplestore);
SetTuplestoreDestReceiverParams(queryDesc->dest,