Re: Add CREATE support to event triggers - Mailing list pgsql-hackers

In the 2nd case though I imagine that those functions in deparse_utility.c are used afterwards... There are a couple of other warning related to SCT_Simple but that's normal as long as 17 or 24 are not combined with it.
3) What's that actually?
+/* XXX merge this with ObjectTypeMap? */
 static event_trigger_support_data event_trigger_support[] = {

We may be able to do something smarter with event_trigger_support[], but as this consists in a mapping of subcommands associated with CREATE/DROP/ALTER and is only a reverse engineering operation of somewhat AlterObjectTypeCommandTag  or CreateCommandTag, I am not sure how you could merge that. Some input perhaps?
4)
+/*
+ * EventTriggerStashCommand
+ *             Save data about a simple DDL command that was just executed
+ */

Shouldn't this be "single" instead of "simple"?

5) I think that SCT_Simple should be renamed as SCT_Single

+typedef enum StashedCommandType
+{
+       SCT_Simple,
+} StashedCommandType;

This comment holds as well for deparse_simple_command.

6)

+               command = deparse_utility_command(cmd);
+
+               /*
+                * Some parse trees return NULL when deparse is attempted; we don't
+                * emit anything for them.
+                */
+               if (command != NULL)
+               {

Small detail, but you may here just use a continue to make the code a bit more readable after deparsing the command.
7) pg_event_trigger_get_creation_commands is modified as well in patches 17 and 24. You may as well use an enum on cmd->type.

8) Rejoining a comment done earlier by Andres, it would be nice to have ERRCODE_WRONG_CONTEXT (unrelated to this patch). ERRCODE_FEATURE_NOT_SUPPORTED seems rather a different error type...
9) Those declarations are not needed in event_trigger.c:
+#include "utils/json.h"
+#include "utils/jsonb.h"

10) Would you mind explaining what means "fmt"?
+ * Allocate a new object tree to store parameter values -- varargs version.
+ *
+ * The "fmt" argument is used to append as a "fmt" element in the output blob.

11) deparse_utility.c mentions here and there JSON objects, but what is created are JSONB objects. I'd rather be clear here.

12) Already mentioned before, but this reverse engineering machine for types would be more welcome as a set of functions in lsyscache (one for namespace, one for type name and one for is_array). For typemodstr the need is different as it uses printTypmod.

+void
+format_type_detailed(Oid type_oid, int32 typemod,
+                                        Oid *nspid, char **typname, char **typemodstr,
+                                        bool *is_array)

13) This change seems unrelated to this patch...
-       int                     type = 0;
+       JsonbIteratorToken type = WJB_DONE;
        JsonbValue      v;
        int                     level = 0;
        bool            redo_switch = false;
@@ -454,7 +454,7 @@ JsonbToCString(StringInfo out, JsonbContainer *in, int estimated_len)
                                first = false;
                                break;
                        default:
-                               elog(ERROR, "unknown flag of jsonb iterator");
+                               elog(ERROR, "unknown jsonb iterator token type");

14) This could already be pushed as a separate commit:

-extern bool creating_extension;
+extern PGDLLIMPORT bool creating_extension;

I am actually a bit doubtful about why we actually need this intermediate ObjTree step... What would be wrong in manipulating a JSON(B) parsed tree, that is first empty, and pushing key/value objects to it when processing each command? Something moving toward in this direction is that ObjTree has some logic to manipulate booleans, null values, etc. This results in some duplication with what jsonb and json can actually do when creating when manipulating strings, as well as the extra processing like objtree_to_jsonb_element. ddl_json.c (should be ddl_jsonb.c?) would as well take more its sense as it directly manipulates JSONB containers.

Regards,

--
Michael