Обсуждение: snapshot leak and core dump with serializable transactions

The following test flashes snapshot leak warning and subsequently dumps core. Though this looks very similar to other bug report, this is a different issue.


postgres=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE ;
BEGIN
postgres=# SAVEPOINT A;
SAVEPOINT
postgres=# SELECT count(*) from pg_class;
 count
-------
   227
(1 row)

postgres=# RELEASE SAVEPOINT A;
WARNING:  Snapshot reference leak: Snapshot 0x9e3e4d4 still referenced
RELEASE
postgres=# SELECT count(*) from pg_class;The connection to the server was lost. Attempting reset: Failed.
!>                            I looked at this briefly and ISTM that there are couple of issues here:

1. Since "SAVEPOINT A" is the first statement in the transaction, a subtransaction is started and CurrentResourceOwner is set to the resource owner of the subtransaction. Later when serializable snapshot is taken, its recorded in the subtransaction resource owner. Obviously, when the subtransaction commits, it complains about the snapshot leak because the serializable snapshot is not yet unregiste red.

So I tried to ensure that the serializable snapshot is always recorded in the TopTransactionResourceOwner. It solved the above issue, but there is still a core dump when the top transaction is committed. That leads to the second issue.

2. In CommitTransaction(), I think we should call AtEOXact_Snapshot *before* releasing the resource owners. Otherwise, ResourceOwnerReleaseInternal complains about snapshot leak and then forcefully unregisters the snapshot. Later when AtEOXact_Snapshot is called, it again tries to unregister the serializable snapshot and assertion fails.

The attached patch fixes these issues.

Thanks,
Pavan


--
Pavan Deolasee
EnterpriseDB     http://www.enterprisedb.com

Pavan Deolasee escribió:

> 2. In CommitTransaction(), I think we should call AtEOXact_Snapshot *before*
> releasing the resource owners. Otherwise, ResourceOwnerReleaseInternal
> complains about snapshot leak and then forcefully unregisters the snapshot.
> Later when AtEOXact_Snapshot is called, it again tries to unregister the
> serializable snapshot and assertion fails.

Hmm, I've been wondering if we can get away with not having
AtEOXact_Snapshot at all.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> 2. In CommitTransaction(), I think we should call AtEOXact_Snapshot *before*
> releasing the resource owners.

That's absolutely wrong.  It'll complain about whatever snapshots the
owners still hold.
        regards, tom lane

<br /><br /><div class="gmail_quote">On Wed, Dec 3, 2008 at 7:42 PM, Tom Lane <span dir="ltr"><<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>></span>wrote:<br /><blockquote class="gmail_quote"
style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br
/><br/></div>That's absolutely wrong.  It'll complain about whatever snapshots the<br /> owners still hold.<br /><br
/></blockquote></div><br/>You must be right; I don't understand that code much. But don't we expect the snapshots to be
cleanlyreleased at that point and if not we flash warnings anyways ? AtEOXact_Snapshot only unregisters the serialized
snapshotwhich otherwise release resource owner will complain about.<br /><br clear="all" />Thanks,<br />Pavan<br /><br
/>--<br />Pavan Deolasee<br />EnterpriseDB     <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br
/>

Pavan Deolasee escribió:
> On Wed, Dec 3, 2008 at 7:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > That's absolutely wrong.  It'll complain about whatever snapshots the
> > owners still hold.
>
> You must be right; I don't understand that code much. But don't we expect
> the snapshots to be cleanly released at that point and if not we flash
> warnings anyways ? AtEOXact_Snapshot only unregisters the serialized
> snapshot which otherwise release resource owner will complain about.

Yeah, we need two "at-commit" routines, one of which needs to be called
early.  I'm prepping a patch.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera escribió:

> Yeah, we need two "at-commit" routines, one of which needs to be called
> early.  I'm prepping a patch.

Here it is ... the large object patch is also included.  I've created
new functions to specify the resource owner to register a snapshot in;
now that there are two callers, it seems likely that there will be more
in the future.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Alvaro Herrera escribió:
> 
>> Yeah, we need two "at-commit" routines, one of which needs to be called
>> early.  I'm prepping a patch.
> 
> Here it is ... the large object patch is also included.  I've created
> new functions to specify the resource owner to register a snapshot in;
> now that there are two callers, it seems likely that there will be more
> in the future.

I'm surprised you implemented RegisterSnapshotOnOwner by switching 
CurrentResourceOwner and calling RegisterSnapshot, rather than 
implementing RegisterSnapshot by calling RegisterSnapshotOnOwner(..., 
CurrentResourceOwner).

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas escribió:

> I'm surprised you implemented RegisterSnapshotOnOwner by switching
> CurrentResourceOwner and calling RegisterSnapshot, rather than
> implementing RegisterSnapshot by calling RegisterSnapshotOnOwner(...,
> CurrentResourceOwner).

Yeah, that was plenty silly.  Updated patch attached.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Looks good me to, except for this warning:

snapmgr.c: In function 'RegisterSnapshot':
snapmgr.c:356: warning: unused variable 'snap'

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB     http://www.enterprisedb.com

Pavan Deolasee escribió:
> On Thu, Dec 4, 2008 at 2:25 AM, Alvaro Herrera
> <alvherre@commandprompt.com>wrote:
> 
> > Yeah, that was plenty silly.  Updated patch attached.
>
> Looks good me to, except for this warning:

Applied.  Many thanks for the exhaustive testing.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.