Re: Add CREATE support to event triggers

Hi Michael, thanks for the review.

Michael Paquier wrote:
> 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.

Agreed.

> 2) This patch is showing up many warnings, among them:

Will check.

> 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?

ObjectTypeMap is part of the patch that handles DROP for replication;
see my other patch in commitfest. I am also not sure how to merge all
this stuff; with these patches, we would have three "do event triggers
support this object type" functions, so I was thinking in having maybe a
text file from which these functions are generated from a perl script or
something. But for now I think it's okay to keep things like this.
That comment is only there to remind me that some cleanup might be in
order.

> 4)
> +/*
> + * EventTriggerStashCommand
> + * Save data about a simple DDL command that was just executed
> + */
> Shouldn't this be "single" instead of "simple"?

In an older version it was "basic". Not wedded to "simple", but I don't
think "single" is the right thing. A later patch in the series
introduces type Grant, and there's also type AlterTable. The idea
behind Simple is to include command types that do not require special
handling; but all these commands are single commands.

> 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.

Will check.

> 9) Those declarations are not needed in event_trigger.c:
> +#include "utils/json.h"
> +#include "utils/jsonb.h"

Will check. I split ddl_json.c at the last minute and I may have
forgotten to remove these.

> 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.

"fmt" is equivalent to sprintf and friends' fmt argument. I guess this
commands needs to be expanded a bit.

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

Good point.

> 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)

I am unsure about this. Other things that require many properties of
the same object do a single lookup and return all of them in a single
call, rather than repeated calls.

> 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");

Yes, sorry. I was trying to figure out how to use the jsonb stuff and
I found this error message was quite unclear. In general, jsonb code
seems to have random warts ...

> 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.

Uhm. Obviously we didn't have jsonb when I started this and we do have
them now, so I could perhaps see about updating the patch to do things
this way; but I'm not totally sold on that idea, as my ObjTree stuff is
a lot easier to manage and the jsonb API is pretty ugly.

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