Re: deparsing utility commands

Andres Freund wrote:
> Hi,
>
> On 2015-02-15 01:48:15 -0300, Alvaro Herrera wrote:
> > This is a repost of the patch to add CREATE command deparsing support to
> > event triggers.  It now supports not only CREATE but also ALTER and
> > other commands such as GRANT/REVOKE, COMMENT ON and SECURITY LABEL.
> > This patch series is broken up in a rather large number of patches, but
> > my intention would be to commit separately only the first 6 patches; the
> > remaining parts are split up only so that it's easy to see how deparsing
> > each patch is implemented, and would be committed as a single changeset
> > (but before that I need a bunch of extra fixes and additions to other
> > parts.)
>
> I find the split of the patches not really helpful - it makes
> fixing/cleaning up things unnecessarily complicated. I'd like most of
> the individual command commits. Obviously the preparatory stuff that'll
> be independely committed should stay separate. I also like that the
> infrastructure patch is split of; same with the more wildly different
> patches like support for GRANT.

Here's a new series.  I have reduced the number of patches, per your
request.  (Of course, a number of those preparatory commits have gotten
pushed.)

One thing that Stephen commented on was the ALTER TABLE preparatory
patch.  He said why not return ObjectAddress from the subcommand
routines instead of just Oid/attnum?  The reason is that to interpret
the address correctly you will still need a lot more context; the OID
and attnum are only part of the truth anyway.  I think there are a small
number of cases where we could meaningfully return an ObjectAddress and
have the whole thing be useful, but I'm not sure it's worth the bother.

In patch 0004, I removed most of the Stash() calls in ProcessUtility,
instead adding one at the bottom that takes care of most of the simple
cases.  That means that a developer adding support for something new in
ProcessUtilitySlow without realizing that something must be added to the
command stash will get an error fairly early, which I think is helpful.

Patch 0021 (fixing a bug in SECURITY LABEL support) I'm not really sure
about.  I don't like modifying a parse node during execution -- seems a
bad habit.  It seems better to me to return the chosen security label as
an ObjectAddress in ExecSecLabelStmt, as pass that as "secondaryOid" to
the deparse_utility.c routine.

In patch 0023 (CREATE/ALTER POLICY support) I ran into trouble.  I
represent the role in the json like this:
    tmp = new_objtree_VA("TO %{role:, }I", 0);
which means that role names are quoted as identifiers.  This means it
doesn't work as soon as we get a command referencing PUBLIC (which many
in the regression test do, because when the TO clause is omitted, PUBLIC
is the default).  So this ends up as "PUBLIC" and everything goes
downhill.  I'm not sure what to do about this, except to invent a new
%{} code.

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