On 4/9/15 12:14 PM, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>> Executive summary:
>>
>> There is now a CommandDeparse_hook;
>> deparse_utility_command is provided as an extension, intended for 9.6;
>> rest of patch would be pushed to 9.5.
>
> Actually here's another approach I like better: use a new pseudotype,
> pg_ddl_command, which internally is just a pointer to a CollectedCommand
> struct. We cannot give access to the pointer directly in SQL, so much
> like type internal or pg_node_tree the in/out functions should just
> error out. But the type can be returned as a column in
> pg_event_trigger_ddl_command. An extension can create a function that
> receives that type as argument, and return a different representation of
> the command; the third patch creates a function ddl_deparse_to_json()
> which does that.
I've tested this approach and it returns the same results as the
previous approach for all calls to pg_event_trigger_ddl_command() made
by the pg_audit regression tests. For my testing I was generally only
applying patch 0001 and 0002 (although 0001 on it's own also worked for
my case). I applied patch 0003 only for the purpose of being sure it
did not break anything, and did not specifically test the functionality
of 0002 or 0003.
However, I've tested 0001 over a very wide range of commands and have
not found any anomalous behaviors.
I've reviewed the 0001 and 0002 patches and don't have anything to add
beyond what has been mentioned in previous reviews. You've applied your
pattern consistently across a wide variety of utility commands which is
something of a feat.
Since 0003 will not be committed to core, I was more interested in the
mechanism used to connect 0003 to 0001 and 0002. I like the new
approach better than the old one in that there is no connecting hook
now. As you say, it provides more flexibility in terms of using the
command data in as many ways as you like. Even better, the user has
access to three different representations and can choose which one is
best for them. And, of course, they can write code for their own
representation using pg_ddl_command, JSON, or SQL.
I also like the way the patch has been broken up. The core
functionality will finally make event triggers truly useful in a
high-level language like pl/pgsql. I can see many uses for them now
that pg_event_trigger_ddl_command() is present.
From my review and testing, I see no barriers to committing patches 0001
and 0002. I'd love to see this functionality in 9.5.
Regards,
--
- David Steele
david@pgmasters.net
