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

From Michael Paquier
Subject Re: Add CREATE support to event triggers
Date
Msg-id CAB7nPqQ_J2MwTmia2rELEYVcSR+8Qs62sPH5X9jdW5=KtO0Fmg@mail.gmail.com
Whole thread Raw
In response to Re: Add CREATE support to event triggers  (Michael Paquier <michael.paquier@gmail.com>)
Responses Re: Add CREATE support to event triggers
List pgsql-hackers
On Thu, Oct 16, 2014 at 4:01 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Here's a new version of this series.
 Here is some input on patch 4:

1) Use of OBJECT_ATTRIBUTE:
+               case OBJECT_ATTRIBUTE:
+                       catalog_id = TypeRelationId;    /* XXX? */
+                       break;
I think that the XXX mark could be removed, using TypeRelationId is correct IMO as OBJECT ATTRIBUTE is used when managing an attribute object, which is a custom type used for CREATE/ALTER TYPE.
2) This patch is showing up many warnings, among them:
event_trigger.c:1460:20: note: uninitialized use occurs here
                                addr.classId = classId;
                                               ^~~~~~~
event_trigger.c:1446:21: note: initialize the variable 'objSubId' to silence this warning
                                uint32          objSubId;
                                                        ^
                                                         = 0
Or:
deparse_utility.c:301:1: warning: unused function 'append_object_object' [-Wunused-function]
append_object_object(ObjTree *tree, char *name, ObjTree *value)
^
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;

A couple of comments: this patch introduces a basic infrastructure able to do the following set of operations:
- Obtention of parse tree using StashedCommand
- Reorganization of parse tree to become an ObjTree, with boolean, array
- Reorganization of ObjTree to a JsonB value
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

pgsql-hackers by date:

Previous
From: Adam Brightwell
Date:
Subject: Re: alter user/role CURRENT_USER
Next
From: "Syed, Rahila"
Date:
Subject: Re: [REVIEW] Re: Compression of full-page-writes