Hi Michael, thanks for the review. Michael Paquier wrote: > On Thu, Oct 16, 2014 at 4:01 PM, Michael Paquier <michael.paqu...@gmail.com> > wrote: > > > On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera > > <alvhe...@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.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers