Re: Command Triggers patch v18

On Friday, March 23, 2012 04:32:02 PM Dimitri Fontaine wrote:
> I would like to get back on code level review now if at all possible,
> and I would integrate your suggestions here into the next patch revision
> if another one is needed.
Ok, I will give it another go.

Btw I just wanted to alert you to being careful when checking in the expect
files ;)

NOTICE: snitch: BEFORE any DROP TRIGGER
-ERROR: unexpected name list length (3)
+NOTICE: snitch: BEFORE DROP TRIGGER <NULL> foo_trigger
+NOTICE: snitch: AFTER any DROP TRIGGER
create conversion test for 'utf8' to 'sjis' from utf8_to_sjis;
j

you had an apparerently un-noticed error in there ;)

1.
if (!HeapTupleIsValid(tup))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("command trigger \"%s\" does not exist, skipping",
trigname)));
The "skipping" part looks like a copy/pasto...

2.
In PLy_exec_command_trigger youre doing a PG_TRY() which looks pointless in
the current incarnation. Did you intend to add something in the catch?
I think without doing a decref of pltdata both in the sucess and the failure
path youre leaking memory.

3.
In plpython: Why do you pass objectId/pltobjectname/... as "NULL" instead of
None? Using a string for it seems like a bad from of in-band signalling to me.

4.
Not sure whether InitCommandContext is the best place to suppress command
trigger usage for some commands. That seems rather unintuitive to me. But
perhaps the implementation-ease is big enough...

Thats everything new I found... Not bad I think. After this somebody else
should take a look at I think (commiter or not).

> The only point yet to address from last round from Andres is about the
> API around CommandFiresTrigger() and the Memory Context we use here.
> We're missing an explicit Reset call, and to be able to have we need to
> have a more complex API, because of the way RemoveObjects() and
> RemoveRelations() work.
>
> We would need to add no-reset APIs and an entry point to manually reset
> the memory context, which currently gets disposed at the same time as
> its parent context, the current one that's been setup before entering
> standard_ProcessUtility().
Not sure if youre expecting further input from me about that?

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services