Nested transactions: deferred triggers

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)

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