Re: sql_drop Event Triggerg

> Robert Haas escribió:

> > Or at least move the save/restore logic into something inside the
> > deletion machinery itself so that new callers don't have to worry
> > about it?

I don't think that works well; precisely the point of the
initialize/finalize pair of functions is to bracket the drop so that the
objects reported by the deletion machinery are stored in the right list.

I tried this macro:

/*
* Wrap a code fragment that executes a command that may drop database objects,
* so that the event trigger environment is appropriately setup.
*
* Note this macro will call EventTriggerDDLCommandEnd if the object type is
* supported; caller must make sure to call EventTriggerDDLCommandStart by
* itself.
*/
#define ExecuteDropCommand(isCompleteQuery, codeFragment, has_objtype, objtype) \
do { \
slist_head _save_objlist; \
bool _supported; \
\
_supported = has_objtype ? EventTriggerSupportsObjectType(objtype) : true; \
\
if (isCompleteQuery) \
{ \
EventTriggerInitializeDrop(&_save_objlist); \
} \
PG_TRY(); \
{ \
codeFragment; \
if (isCompleteQuery && _supported) \
{ \
EventTriggerDDLCommandEnd(parsetree); \
} \
} \
PG_CATCH(); \
{ \
if (isCompleteQuery && _supported) \
{ \
EventTriggerFinalizeDrop(_save_objlist); \
} \
PG_RE_THROW(); \
} \
PG_END_TRY(); \
EventTriggerFinalizeDrop(_save_objlist); \
} while (0)

This looks nice in DropOwned:

case T_DropOwnedStmt:
if (isCompleteQuery)
EventTriggerDDLCommandStart(parsetree);
ExecuteDropCommand(isCompleteQuery,
DropOwnedObjects((DropOwnedStmt *) parsetree),
false, 0);
break;

And it works for DropStmt too:

ExecuteDropCommand(isCompleteQuery,
switch (stmt->removeType)
{
case OBJECT_INDEX:
case OBJECT_TABLE:
case OBJECT_SEQUENCE:
case OBJECT_VIEW:
case OBJECT_MATVIEW:
case OBJECT_FOREIGN_TABLE:
RemoveRelations((DropStmt *) parsetree);
break;
default:
RemoveObjects((DropStmt *) parsetree);
break;
}, true, stmt->removeType);

but a rather serious problem is that pgindent refuses to work completely
on this (which is understandable, IMV). My editor doesn't like the
braces inside something that looks like a function call, either. We use
this pattern (a codeFragment being called by a macro) as
ProcessMessageList in inval.c, but the code fragments there are much
simpler.

And in AlterTable, using the macro would be much uglier because the code
fragment is more convoluted.

I'm not really sure if it's worth having the above macro if the only
caller is DropOwned.

Hmm, maybe I should be considering a pair of macros instead --
UTILITY_START_DROP and UTILITY_END_DROP. I'll give this a try. Other
ideas are welcome.

FWIW, I noticed that the AlterTable case lacks a call to DDLCommandEnd;
reporting that to Dimitri led to him noticing that DefineStmt also lacks
one. This is a simple bug in the already-committed implementation which
should probably be fixed separately from this patch.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services