Removing savepointLevel from TransactionState

Started by Gurjeet Singhover 14 years ago6 messages

singh.gurjeet@gmail.com

over 14 years ago

1 attachment(s)

I noticed that the savepointLevel member of TransactionStateData struct is
initialized to 0 from TopTransactionStateData, and never incremented or
decremented afterwards.

Since this is a file-local struct I think we can simply get rid of all
usages of this without any risk. I visited all the commits where this
variable was introduced/used/changed and could not find any point in history
where it was ever useful. Maybe I missed something, but looks like it is a
leftover from someone's forward-thinking.

Patch attached.

Regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Attachments:

remove_savepointLevel.patchtext/x-diff; charset=US-ASCII; name=remove_savepointLevel.patchDownload

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 3dab45c..cde3772 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -132,7 +132,6 @@ typedef struct TransactionStateData
 	TransactionId transactionId;	/* my XID, or Invalid if none */
 	SubTransactionId subTransactionId;	/* my subxact ID */
 	char	   *name;			/* savepoint name, if any */
-	int			savepointLevel; /* savepoint level */
 	TransState	state;			/* low-level state */
 	TBlockState blockState;		/* high-level state */
 	int			nestingLevel;	/* transaction nesting depth */
@@ -2641,12 +2640,10 @@ CommitTransactionCommand(void)
 		case TBLOCK_SUBRESTART:
 			{
 				char	   *name;
-				int			savepointLevel;
 
 				/* save name and keep Cleanup from freeing it */
 				name = s->name;
 				s->name = NULL;
-				savepointLevel = s->savepointLevel;
 
 				AbortSubTransaction();
 				CleanupSubTransaction();
@@ -2654,7 +2651,6 @@ CommitTransactionCommand(void)
 				DefineSavepoint(NULL);
 				s = CurrentTransactionState;	/* changed by push */
 				s->name = name;
-				s->savepointLevel = savepointLevel;
 
 				/* This is the same as TBLOCK_SUBBEGIN case */
 				AssertState(s->blockState == TBLOCK_SUBBEGIN);
@@ -2670,19 +2666,16 @@ CommitTransactionCommand(void)
 		case TBLOCK_SUBABORT_RESTART:
 			{
 				char	   *name;
-				int			savepointLevel;
 
 				/* save name and keep Cleanup from freeing it */
 				name = s->name;
 				s->name = NULL;
-				savepointLevel = s->savepointLevel;
 
 				CleanupSubTransaction();
 
 				DefineSavepoint(NULL);
 				s = CurrentTransactionState;	/* changed by push */
 				s->name = name;
-				s->savepointLevel = savepointLevel;
 
 				/* This is the same as TBLOCK_SUBBEGIN case */
 				AssertState(s->blockState == TBLOCK_SUBBEGIN);
@@ -3536,12 +3529,6 @@ ReleaseSavepoint(List *options)
 				(errcode(ERRCODE_S_E_INVALID_SPECIFICATION),
 				 errmsg("no such savepoint")));
 
-	/* disallow crossing savepoint level boundaries */
-	if (target->savepointLevel != s->savepointLevel)
-		ereport(ERROR,
-				(errcode(ERRCODE_S_E_INVALID_SPECIFICATION),
-				 errmsg("no such savepoint")));
-
 	/*
 	 * Mark "commit pending" all subtransactions up to the target
 	 * subtransaction.	The actual commits will happen when control gets to
@@ -3635,12 +3622,6 @@ RollbackToSavepoint(List *options)
 				(errcode(ERRCODE_S_E_INVALID_SPECIFICATION),
 				 errmsg("no such savepoint")));
 
-	/* disallow crossing savepoint level boundaries */
-	if (target->savepointLevel != s->savepointLevel)
-		ereport(ERROR,
-				(errcode(ERRCODE_S_E_INVALID_SPECIFICATION),
-				 errmsg("no such savepoint")));
-
 	/*
 	 * Mark "abort pending" all subtransactions up to the target
 	 * subtransaction.	The actual aborts will happen when control gets to
@@ -4287,7 +4268,6 @@ PushTransaction(void)
 	s->parent = p;
 	s->nestingLevel = p->nestingLevel + 1;
 	s->gucNestLevel = NewGUCNestLevel();
-	s->savepointLevel = p->savepointLevel;
 	s->state = TRANS_DEFAULT;
 	s->blockState = TBLOCK_SUBBEGIN;
 	GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);

Tom Lane

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Gurjeet Singh (#1)

Re: Removing savepointLevel from TransactionState

Gurjeet Singh <singh.gurjeet@gmail.com> writes:

I noticed that the savepointLevel member of TransactionStateData struct is
initialized to 0 from TopTransactionStateData, and never incremented or
decremented afterwards.

Since this is a file-local struct I think we can simply get rid of all
usages of this without any risk.

ISTM you have detected a bug, not just dead code that should be removed.
Surely those tests that throw error on savepointLevel change were
meant to do something important?

regards, tom lane

Gurjeet Singh

singh.gurjeet@gmail.com

over 14 years ago

In reply to: Tom Lane (#2)

Re: Removing savepointLevel from TransactionState

On Thu, Sep 29, 2011 at 1:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Gurjeet Singh <singh.gurjeet@gmail.com> writes:

I noticed that the savepointLevel member of TransactionStateData struct

is

initialized to 0 from TopTransactionStateData, and never incremented or
decremented afterwards.

Since this is a file-local struct I think we can simply get rid of all
usages of this without any risk.

ISTM you have detected a bug, not just dead code that should be removed.
Surely those tests that throw error on savepointLevel change were
meant to do something important?

That's surprising for dead-code removal!

Not sure which failures you're pointing to. `make check` before and after
the patch on master says 'All 126 tests passed.'

Let me know about the failures, and I'll see if I can find the root cause.

Regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Pavan Deolasee

pavan.deolasee@gmail.com

over 14 years ago

In reply to: Gurjeet Singh (#3)

Re: Removing savepointLevel from TransactionState

On Thu, Sep 29, 2011 at 10:52 AM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:

On Thu, Sep 29, 2011 at 1:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Gurjeet Singh <singh.gurjeet@gmail.com> writes:

I noticed that the savepointLevel member of TransactionStateData struct
is
initialized to 0 from TopTransactionStateData, and never incremented or
decremented afterwards.

Since this is a file-local struct I think we can simply get rid of all
usages of this without any risk.

ISTM you have detected a bug, not just dead code that should be removed.
Surely those tests that throw error on savepointLevel change were
meant to do something important?

That's surprising for dead-code removal!

Not sure which failures you're pointing to. `make check` before and after
the patch on master says 'All 126 tests passed.'

I think Tom is talking about the test in the code and not a regression
test itself. Looking at the archives and git log, it seems though that
we never completed this feature and the code was always dead since its
inception.

http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg48321.html

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

Alvaro Herrera

alvherre@commandprompt.com

over 14 years ago

In reply to: Tom Lane (#2)

Re: Removing savepointLevel from TransactionState

Excerpts from Tom Lane's message of jue sep 29 02:11:52 -0300 2011:

Gurjeet Singh <singh.gurjeet@gmail.com> writes:

I noticed that the savepointLevel member of TransactionStateData struct is
initialized to 0 from TopTransactionStateData, and never incremented or
decremented afterwards.

Since this is a file-local struct I think we can simply get rid of all
usages of this without any risk.

ISTM you have detected a bug, not just dead code that should be removed.
Surely those tests that throw error on savepointLevel change were
meant to do something important?

This is the patch I submitted that introduced that bit:
http://archives.postgresql.org/pgsql-patches/2004-07/msg00292.php
which got committed as cc813fc2b8d9293bbd4d0e0d6a6f3b9cf02fe32f.

Amusingly, the savepointLevel thing was introduced there; I don't
remember the details but I think what it was intended to implement is
some sort of restriction laid out by the SQL standard's spelling of
savepoint commands.

... in fact, SQL 2008 talks about savepoint levels in "4.35.2
Savepoints". And as far as "Part 2: Foundation" is concerned, I think
only <routine invocation> can cause the savepoint level to be changed.
That is, if you have a function that declares itself to have NEW SAVEPOINT
LEVEL, then that function is not allowed to roll back savepoints that
were created before it started.

Now, we already disallow functions from doing this at all; so it seems
that the missing feature for us is OLD SAVEPOINT LEVEL (which, according
to the standard, is the default behavior). Since this is not
implementable on the current SPI abstraction, we cannot do much about
this. But if we ever have transaction-controlling SPs, then it seems to
me that we ought to keep this and enable those to use it as appropriate.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Gurjeet Singh

singh.gurjeet@gmail.com

over 14 years ago

In reply to: Alvaro Herrera (#5)

Re: Removing savepointLevel from TransactionState

On Thu, Sep 29, 2011 at 8:10 AM, Alvaro Herrera
<alvherre@commandprompt.com>wrote:

Excerpts from Tom Lane's message of jue sep 29 02:11:52 -0300 2011:

Gurjeet Singh <singh.gurjeet@gmail.com> writes:

I noticed that the savepointLevel member of TransactionStateData struct

is

initialized to 0 from TopTransactionStateData, and never incremented or
decremented afterwards.

Since this is a file-local struct I think we can simply get rid of all
usages of this without any risk.

ISTM you have detected a bug, not just dead code that should be removed.
Surely those tests that throw error on savepointLevel change were
meant to do something important?

This is the patch I submitted that introduced that bit:
http://archives.postgresql.org/pgsql-patches/2004-07/msg00292.php
which got committed as cc813fc2b8d9293bbd4d0e0d6a6f3b9cf02fe32f.

Amusingly, the savepointLevel thing was introduced there; I don't
remember the details but I think what it was intended to implement is
some sort of restriction laid out by the SQL standard's spelling of
savepoint commands.

... in fact, SQL 2008 talks about savepoint levels in "4.35.2
Savepoints". And as far as "Part 2: Foundation" is concerned, I think
only <routine invocation> can cause the savepoint level to be changed.
That is, if you have a function that declares itself to have NEW SAVEPOINT
LEVEL, then that function is not allowed to roll back savepoints that
were created before it started.

Now, we already disallow functions from doing this at all; so it seems
that the missing feature for us is OLD SAVEPOINT LEVEL (which, according
to the standard, is the default behavior). Since this is not
implementable on the current SPI abstraction, we cannot do much about
this. But if we ever have transaction-controlling SPs, then it seems to
me that we ought to keep this and enable those to use it as appropriate.

I have seen some recent discussions around implementing procedures that
would allow transaction control, but don't know at what stage those
conversations ended. If we are still at hand-waving stage w.r.t SPs then I
would vote for removal of this code and rethink it as part of SP
implementation.

Having seen some commits after the initial one, that use this variable, ISTM
that we're maintaining a feature we never documented, or implemented for
that matter.

Dead code trips the unwary like me, and definitely does not help a
maintainer.

Regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company