Thread: Snapshot warning

<br />Following test case gives a warning of snapshot not destroyed at commit time.<br /><br />CREATE TABLE test (a
int);<br />INSERT INTO test VALUES (1);<br />BEGIN;<br /> DECLARE c CURSOR FOR SELECT * FROM test FOR update;<br />
SAVEPOINTA;<br /> FETCH -2 FROM c;<br /> ROLLBACK TO SAVEPOINT A;<br /> COMMIT;<br /><br />Should we call
FreeQueryDesc()even for failed portals in PortalCleanup() ? Or PortalDrop() is a better(right) place to do that ?<br
/><br/>Thanks,<br />Pavan<br clear="all" /><br />-- <br />Pavan Deolasee<br />EnterpriseDB     <a
href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/> 

Pavan Deolasee escribió:
> Following test case gives a warning of snapshot not destroyed at commit
> time.
> 
> CREATE TABLE test (a int);
> INSERT INTO test VALUES (1);
> BEGIN;
> DECLARE c CURSOR FOR SELECT * FROM test FOR update;
> SAVEPOINT A;
> FETCH -2 FROM c;
> ROLLBACK TO SAVEPOINT A;
> COMMIT;
> 
> Should we call FreeQueryDesc() even for failed portals in PortalCleanup() ?
> Or PortalDrop() is a better(right) place to do that ?

That doesn't work; doing it causes a crash:

TRAP: FailedAssertion(«!(qdesc->estate == ((void *)0))», Archivo: «/pgsql/source/00head/src/backend/tcop/pquery.c»,
Línea:126)
 

which is here:

void
FreeQueryDesc(QueryDesc *qdesc)
{   /* Can't be a live query */   Assert(qdesc->estate == NULL);

BTW I noticed that AtSubAbort_Portals() is neglecting to set
portal->cleanup to NULL after calling it.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Pavan Deolasee escribi�:
>> Should we call FreeQueryDesc() even for failed portals in PortalCleanup() ?
>> Or PortalDrop() is a better(right) place to do that ?

> That doesn't work; doing it causes a crash:

I think the fundamental bug here is that you tried to skip using the
ResourceOwner mechanism for snapshot references.  That's basically
not gonna work.
        regards, tom lane

Tom Lane escribió:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Pavan Deolasee escribi�:
> >> Should we call FreeQueryDesc() even for failed portals in PortalCleanup() ?
> >> Or PortalDrop() is a better(right) place to do that ?
> 
> > That doesn't work; doing it causes a crash:
> 
> I think the fundamental bug here is that you tried to skip using the
> ResourceOwner mechanism for snapshot references.  That's basically
> not gonna work.

Right :-(  I'll see how to go about this.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane escribió:
>> I think the fundamental bug here is that you tried to skip using the
>> ResourceOwner mechanism for snapshot references.  That's basically
>> not gonna work.

> Right :-(  I'll see how to go about this.

It strikes me that you might be able to remove the registered-snapshot
infrastructure in snapmgr.c, and end up with about the same amount of
code overall, just with the responsibility in resowner.c.
        regards, tom lane

Tom Lane escribió:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Tom Lane escribió:
> >> I think the fundamental bug here is that you tried to skip using the
> >> ResourceOwner mechanism for snapshot references.  That's basically
> >> not gonna work.
> 
> > Right :-(  I'll see how to go about this.
> 
> It strikes me that you might be able to remove the registered-snapshot
> infrastructure in snapmgr.c, and end up with about the same amount of
> code overall, just with the responsibility in resowner.c.

Seems to work, and fixes Pavan test case as well.

$ runpg 00head showdiff  | diffstatbackend/utils/resowner/resowner.c |   99
++++++++++++++++++++backend/utils/time/snapmgr.c     |  180
++++++-------------!!!!!!!!!!!!!!!!!include/utils/resowner.h         |    8 +include/utils/snapmgr.h           |    3 4
fileschanged, 143 insertions(+), 59 deletions(-), 88 modifications(!)
 

I need to fix some comments before publishing the patch.

The only thing I'm now missing is SnapshotResetXmin().  It currently
looks like this:

static void
SnapshotResetXmin(void)
{if (RegisteredSnapshotList == NULL && ActiveSnapshot == NULL)    MyProc->xmin = InvalidTransactionId;
}

After the patch we don't have any way to detect whether resowner.c has
any snapshot still linked to.  I assume there's no objection to adding a
new entry point in resowner.c for this.


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

Alvaro Herrera escribió:

> The only thing I'm now missing is SnapshotResetXmin().  It currently
> looks like this:


> After the patch we don't have any way to detect whether resowner.c has
> any snapshot still linked to.  I assume there's no objection to adding a
> new entry point in resowner.c for this.

Hmm, that doesn't readily work because there's no way to track
transaction boundaries in resource owners.  I think I'll have to add a
separate static counter in snapmgr.c that's maintained by the calls from
resowner.c.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> After the patch we don't have any way to detect whether resowner.c has
> any snapshot still linked to.  I assume there's no objection to adding a
> new entry point in resowner.c for this.

Hmm, that's a bit problematic because resowner.c doesn't have any global
notion of what resource owners exist.  I think you still need to have
snapmgr.c maintain a list of all known snapshots.  resowner.c can only
help you with tracking reference counts for particular snapshots.
        regards, tom lane

Tom Lane escribió:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > After the patch we don't have any way to detect whether resowner.c has
> > any snapshot still linked to.  I assume there's no objection to adding a
> > new entry point in resowner.c for this.
>
> Hmm, that's a bit problematic because resowner.c doesn't have any global
> notion of what resource owners exist.  I think you still need to have
> snapmgr.c maintain a list of all known snapshots.  resowner.c can only
> help you with tracking reference counts for particular snapshots.

A counter seems to suffice.  Patch attached.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane escribi�:
>> Hmm, that's a bit problematic because resowner.c doesn't have any global
>> notion of what resource owners exist.  I think you still need to have
>> snapmgr.c maintain a list of all known snapshots.  resowner.c can only
>> help you with tracking reference counts for particular snapshots.

> A counter seems to suffice.  Patch attached.

Looks sane to me.  The list solution might be needed later, if we wanted
to get smarter about advancing our xmin after deleting only some of our
snapshots ... but this'll do for now.
        regards, tom lane

Pavan Deolasee escribió:
> Following test case gives a warning of snapshot not destroyed at commit
> time.
> 
> CREATE TABLE test (a int);
> INSERT INTO test VALUES (1);
> BEGIN;
> DECLARE c CURSOR FOR SELECT * FROM test FOR update;
> SAVEPOINT A;
> FETCH -2 FROM c;
> ROLLBACK TO SAVEPOINT A;
> COMMIT;

Fixed, thanks for the test case.

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