Skip site navigation (1) Skip section navigation (2)

Re: Removing savepointLevel from TransactionState

From: Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com>
To: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Removing savepointLevel from TransactionState
Date: 2011-09-29 13:06:11
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
On Thu, Sep 29, 2011 at 8:10 AM, Alvaro Herrera

> Excerpts from Tom Lane's message of jue sep 29 02:11:52 -0300 2011:
> > Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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:
> 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

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

Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

In response to

pgsql-hackers by date

Next:From: MauMauDate: 2011-09-29 13:13:30
Subject: Re: Does RelCache/SysCache shrink except when relations are deleted?
Previous:From: Bruce MomjianDate: 2011-09-29 12:56:09
Subject: Re: pg_upgrade - add config directory setting

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group