Thread: Nested transactions: deferred triggers
Hackers, In an attempt to simplify my life I'm submitting this patch that restructures the deferred trigger queue. The fundamental change is to put all the static variables to hold the deferred triggers in a single structure. This is part of the ongoing nested transactions work but can survive alone. There's no change in functionality and regression tests pass. Please apply. -- Alvaro Herrera (<alvherre[a]dcc.uchile.cl>) "El dia que dejes de cambiar dejaras de vivir"
Attachment
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> In an attempt to simplify my life I'm submitting this patch that
> restructures the deferred trigger queue. The fundamental change is to
> put all the static variables to hold the deferred triggers in a single
> structure.
Seems reasonable, but I have a stylistic gripe:
> + static DeferredTriggers ts;
I dislike static variables with names as short as that --- they are too
likely to conflict against local variables. (And before you say there's
no problem because a local declaration would mask it, what happens if
you forget the local declaration?)
I suspect you named it this way because you intend to pass it as a
parameter to all these routines later, and you're trying to avoid
one pass of editing when you add "DeferredTriggers ts" to the parameter
lists. I would suggest doing that now and including it in the patch.
Whether you are intending that or not, please use a better name for
the static variable.
regards, tom lane
On Wed, Jun 11, 2003 at 03:53:56PM -0400, Tom Lane wrote: > Seems reasonable, but I have a stylistic gripe: > > > + static DeferredTriggers ts; > > I dislike static variables with names as short as that --- they are too > likely to conflict against local variables. (And before you say there's > no problem because a local declaration would mask it, what happens if > you forget the local declaration?) > > I suspect you named it this way because you intend to pass it as a > parameter to all these routines later, and you're trying to avoid > one pass of editing when you add "DeferredTriggers ts" to the parameter > lists. I would suggest doing that now and including it in the patch. > Whether you are intending that or not, please use a better name for > the static variable. Actually, the function itself is going to obtain it via a TransactionGetDeferredTriggers() call so it's going to be a variable local to the function. I'm not sure if it can be made a parameter, because I don't control the deferred trigger queue completely (e.g. on new item insertion the caller is something outside my control). Please comment on what do you think about the TransactionGetDeferredTrigger function mentioned earlier. This is going to be in a struct that will be part of the TransactionState, and which will hold pointer to the deferred triggers queue, the smgr's pending deletes, the asynchronous notify queue and the OnCommit actions queue. Also there will be a MemoryContext for holding items on each of those lists. This context will be destroyed on subtransaction abort but will be kept on subtransaction commit -- this allows for easy deallocation of said items on subtrans abort, while keeping them after the subtransaction commits. So I need a way for the affected subsystems to get their structures. This is it. There'll also be a TransactionGetParentPendingDeletes so I can concatenate the list of the committing subtransaction to its parent's list. (This will be done using your new cool FastLists, BTW). Regarding the current patch, I have changed the variable name to something better. Please apply. Thanks for the quick reviewing. -- Alvaro Herrera (<alvherre[a]dcc.uchile.cl>) "The Gord often wonders why people threaten never to come back after they've been told never to return" (www.actsofgord.com)
Attachment
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
>> Seems reasonable, but I have a stylistic gripe ...
>> I suspect you named it this way because you intend to pass it as a
>> parameter to all these routines later,
> Actually, the function itself is going to obtain it via a
> TransactionGetDeferredTriggers() call so it's going to be a variable
> local to the function. I'm not sure if it can be made a parameter,
My thought would be that TransactionGetDeferredTriggers() ought to be
called in one place that then passes the list pointer to the other
subroutines. I haven't looked at the code in detail to see how this
fits in ... but I don't like the notion of all these little functions
independently fetching a pointer from some nontrivial function. That
opens you up to interesting problems if different functions manage to
fetch different results.
regards, tom lane
On Wed, Jun 11, 2003 at 10:33:07PM -0400, Tom Lane wrote:
> My thought would be that TransactionGetDeferredTriggers() ought to be
> called in one place that then passes the list pointer to the other
> subroutines. I haven't looked at the code in detail to see how this
> fits in ... but I don't like the notion of all these little functions
> independently fetching a pointer from some nontrivial function. That
> opens you up to interesting problems if different functions manage to
> fetch different results.
Well, I can see it getting into serious trouble if something weird
happens. However, I don't know a better way to do this. Code snippets
follow.
typedef struct TransactionSpecialData
{
void *deferredTriggers;
/* some other things... */
} TransactionSpecialData;
typedef struct TransactionStateData
{
TransactionId transactionIdData;
...
/* the usual, and the "Special pointer" : */
TransactionSpecial special;
} TransactionStateData;
The TransactionSpecial is initialized on StartSubTransaction, which is
analogous to StartTransaction but is, of course, only called when a
subtransaction starts. If there's inconsistency in the management of
this, I don't think we have much way out but elog(PANIC) and such. I
don't have checks in place for this though.
When the (sub)transaction starts, DeferredTriggersBeginXact() is called
which does the same as in the posted patch plus a call to
TransactionSetDeferredTriggerQueue(). After that, the routines in
backend/commands/trigger.c just call
TransactionGetDeferredTriggerQueue() to get the current queue for
processing.
This routine is defined simply as:
void *
TransactionGetDeferredTriggerQueue(void)
{
TransactionState s = CurrentTransactionState;
return s->special->deferredTriggers;
}
and the setter is just the trivial opposite.
If there's a better way to do this I'll gladly rewrite this code. I
think the complexity of the mechanism I have in place is very close to
the minimum. One possible idea is to have a global variable that is
changed at subtransaction start... however I think handling that is more
complicated because we need to append some things on subtrans commit but
forget them on subtrans abort, so it's not that trivial.
--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Investigación es lo que hago cuando no sé lo que estoy haciendo"
(Wernher von Braun)
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Alvaro Herrera wrote:
> On Wed, Jun 11, 2003 at 03:53:56PM -0400, Tom Lane wrote:
>
> > Seems reasonable, but I have a stylistic gripe:
> >
> > > + static DeferredTriggers ts;
> >
> > I dislike static variables with names as short as that --- they are too
> > likely to conflict against local variables. (And before you say there's
> > no problem because a local declaration would mask it, what happens if
> > you forget the local declaration?)
> >
> > I suspect you named it this way because you intend to pass it as a
> > parameter to all these routines later, and you're trying to avoid
> > one pass of editing when you add "DeferredTriggers ts" to the parameter
> > lists. I would suggest doing that now and including it in the patch.
> > Whether you are intending that or not, please use a better name for
> > the static variable.
>
> Actually, the function itself is going to obtain it via a
> TransactionGetDeferredTriggers() call so it's going to be a variable
> local to the function. I'm not sure if it can be made a parameter,
> because I don't control the deferred trigger queue completely (e.g. on
> new item insertion the caller is something outside my control).
>
> Please comment on what do you think about the
> TransactionGetDeferredTrigger function mentioned earlier. This is going
> to be in a struct that will be part of the TransactionState, and which
> will hold pointer to the deferred triggers queue, the smgr's pending
> deletes, the asynchronous notify queue and the OnCommit actions queue.
>
> Also there will be a MemoryContext for holding items on each of those
> lists. This context will be destroyed on subtransaction abort but will
> be kept on subtransaction commit -- this allows for easy deallocation of
> said items on subtrans abort, while keeping them after the
> subtransaction commits.
>
> So I need a way for the affected subsystems to get their structures.
> This is it. There'll also be a TransactionGetParentPendingDeletes so I
> can concatenate the list of the committing subtransaction to its
> parent's list. (This will be done using your new cool FastLists, BTW).
>
>
> Regarding the current patch, I have changed the variable name to
> something better. Please apply.
>
> Thanks for the quick reviewing.
>
> --
> Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
> "The Gord often wonders why people threaten never to come back after they've
> been told never to return" (www.actsofgord.com)
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Newest version of this patch applied. Thanks. --------------------------------------------------------------------------- Alvaro Herrera wrote: > Hackers, > > In an attempt to simplify my life I'm submitting this patch that > restructures the deferred trigger queue. The fundamental change is to > put all the static variables to hold the deferred triggers in a single > structure. > > This is part of the ongoing nested transactions work but can survive > alone. There's no change in functionality and regression tests pass. > Please apply. > > -- > Alvaro Herrera (<alvherre[a]dcc.uchile.cl>) > "El dia que dejes de cambiar dejaras de vivir" [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 3: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073