Snapshot warning

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 | diffstat
backend/utils/resowner/resowner.c | 99 ++++++++++++++++++++
backend/utils/time/snapmgr.c | 180 ++++++-------------!!!!!!!!!!!!!!!!!
include/utils/resowner.h | 8 +
include/utils/snapmgr.h | 3
4 files changed, 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