Re: Add CREATE support to event triggers

On Thu, Oct 16, 2014 at 4:01 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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