Thread: DROP and ddl_command_end.

Hello.

When I created an event trigger for ddl_command_end, I think the only
means to identify for what the trigger function is called is
pg_event_trigger_ddl_commands() so I wrote as the following function
and defined an event trigger for ddl_command_end.

CREATE OR REPLACE FUNCTION hoge() RETURNS event_trigger AS $$
DECLARE
  cmd record =  pg_event_trigger_ddl_commands();
BEGIN
  RAISE NOTICE '"%" "%" "%"',
        cmd.command_tag, cmd.object_type, cmd.object_identity;
END
$$ LANGUAGE plpgsql;

CREATE EVENT TRIGGER hoge_trigger ON ddl_command_end EXECUTE FUNCTION hoge();

Finally I got an ERROR while DROP.

=# CREATE TABLE t (a int);
NOTICE:  "CREATE TABLE" "table" "public.t"
CREATE TABLE
postgres=# DROP TABLE t;
ERROR:  record "cmd" is not assigned yet
DETAIL:  The tuple structure of a not-yet-assigned record is indeterminate.
CONTEXT:  PL/pgSQL function hoge() line 5 at RAISE

The function doesn't return a record for DROP statements.

The documentation is written as the follows:

https://postgresql.org/docs/current/event-trigger-definition.html
> The ddl_command_end event occurs just after the execution of this same
> set of commands. To obtain more details on the DDL operations that
> took place, use the set-returning function
> pg_event_trigger_ddl_commands() from the ddl_command_end event trigger
> code (see Section 9.28). Note that the trigger fires after the actions
> have taken place (but before the transaction commits), and thus the
> system catalogs can be read as already changed.

So I think at least pg_event_trigger_ddl_command must return a record
for all commands that trigger ddl_command_end and the record should
have the correct command_tag.  DROP TABLE is currently classified as
supporting event trigger.  If we don't do that, any workaround and
documentation is needed.

I may be missing something, andt any opinions, thoughts or suggestions
are welcome.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 91800d1fac..8376ce429b 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1939,7 +1939,8 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
          * state anyway.
          */
         if (cmd->type == SCT_Simple &&
-            !OidIsValid(cmd->d.simple.address.objectId))
+            !OidIsValid(cmd->d.simple.address.objectId) &&
+            !IsA(cmd->parsetree, DropStmt))
             continue;
 
         MemSet(nulls, 0, sizeof(nulls));
@@ -1952,8 +1953,8 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
             case SCT_CreateOpClass:
             case SCT_AlterTSConfig:
                 {
-                    char       *identity;
-                    char       *type;
+                    char       *identity = NULL;
+                    char       *type = NULL;
                     char       *schema = NULL;
 
                     if (cmd->type == SCT_Simple)
@@ -1969,8 +1970,12 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
                     else if (cmd->type == SCT_AlterTSConfig)
                         addr = cmd->d.atscfg.address;
 
-                    type = getObjectTypeDescription(&addr);
-                    identity = getObjectIdentity(&addr);
+                    if (memcmp(&addr, &InvalidObjectAddress,
+                               sizeof(ObjectAddress)) != 0)
+                    {
+                        type = getObjectTypeDescription(&addr);
+                        identity = getObjectIdentity(&addr);
+                    }
 
                     /*
                      * Obtain schema name, if any ("pg_temp" if a temp
@@ -2023,14 +2028,20 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
                     /* command tag */
                     values[i++] = CStringGetTextDatum(CreateCommandName(cmd->parsetree));
                     /* object_type */
-                    values[i++] = CStringGetTextDatum(type);
+                    if (type != NULL)
+                        values[i++] = CStringGetTextDatum(type);
+                    else
+                        nulls[i++] = true;
                     /* schema */
                     if (schema == NULL)
                         nulls[i++] = true;
                     else
                         values[i++] = CStringGetTextDatum(schema);
                     /* identity */
-                    values[i++] = CStringGetTextDatum(identity);
+                    if (identity != NULL)
+                        values[i++] = CStringGetTextDatum(identity);
+                    else
+                        nulls[i++] = true;
                     /* in_extension */
                     values[i++] = BoolGetDatum(cmd->in_extension);
                     /* command */
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index b1f7f6e2d0..3b801d279a 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1686,8 +1686,8 @@ ProcessUtilitySlow(ParseState *pstate,
 
             case T_DropStmt:
                 ExecDropStmt((DropStmt *) parsetree, isTopLevel);
-                /* no commands stashed for DROP */
-                commandCollected = true;
+                /* Dropped object is not available */
+                address =  InvalidObjectAddress;
                 break;
 
             case T_RenameStmt:

On Mon, Mar 9, 2020 at 3:54 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> I may be missing something, andt any opinions, thoughts or suggestions
> are welcome.

Since it's a set-returning function, I would have expected that
instead of trying to assign the result to a variable, you'd loop over
it using FOR var IN query.

But if that's the problem, the error message is a bit odd.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks.

At Mon, 9 Mar 2020 13:29:47 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Mon, Mar 9, 2020 at 3:54 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > I may be missing something, andt any opinions, thoughts or suggestions
> > are welcome.
> 
> Since it's a set-returning function, I would have expected that
> instead of trying to assign the result to a variable, you'd loop over
> it using FOR var IN query.

Yes, right and I know. I intended the sample being simple, but sorry
for the bogus example. But the problem is not there. The problem is,
the trigger is called for DROP, the function returns no tuples. I'm
not sure DROP is the only command to cause the behavior, but if no
tuples means DROP, we should document that behavior.  Otherwise, we
need to document something  like:

"pg_event_trigger_ddl_commands() may omit some of the commands and may
 return no tuples."

But it is quite odd.

> But if that's the problem, the error message is a bit odd.

The error message is correct if we allow zero-tuple return from the
function.  Is it useful if we return DROP event with more information
than just DROP <OBJTYPE>?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Mar 9, 2020 at 11:54 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> Yes, right and I know. I intended the sample being simple, but sorry
> for the bogus example. But the problem is not there. The problem is,
> the trigger is called for DROP, the function returns no tuples. I'm
> not sure DROP is the only command to cause the behavior, but if no
> tuples means DROP, we should document that behavior.  Otherwise, we
> need to document something  like:
>
> "pg_event_trigger_ddl_commands() may omit some of the commands and may
>  return no tuples."
>
> But it is quite odd.

Well, I'm not sure what you're saying here. It seems like you're
saying the feature is broken. If that's true, instead of documenting
that it doesn't work, shouldn't we fix it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company