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 think you should just go ahead and commit 0001, 0002, 0006. Those have
> previously been discussed and seem like a good idea and uncontroversial
> even if the rest of the series doesn't go in.

I have pushed 0001 (changed pg_identify_object for opclasses and
opfamilies to use USING instead of FOR) to master only, and backpatched
a fix for pg_conversion object identities, which were missing
schema-qualification.

Patch 0002 I think is good to go as well, AFAICT (have the various
RENAME commands return the OID and attnum of affected objects).

On 0006 (which is about having ALTER TABLE return the OID/attnum of the
affected object on each subcommand), I have a problem about the ALTER
TABLE ALTER COLUMN SET DATA TYPE USING subcommand. The problem with
that is that in order to fully deparse that subcommand we need to
deparse the expression of the USING clause. But in the parse node we
only have info about the untransformed expression, so it is not possible
to pass it through ruleutils directly; it needs to be run by
transformExpr() first. Doing that in the deparse code seems the wrong
solution, so I am toying with the idea of adding a "Node *" output
parameter for some ATExec* routines; the Prep step would insert the
transformed expression there, so that it is available at the deparse
stage. As far as I know, only the SET DATA TYPE USING form has this
issue: for other subcommands, the current code is simply grabbing the
expression from catalogs. Maybe it would be good to use that Node in
those cases as well and avoid catalog lookups, not sure.

> I think the GRANT/REVOKE, COMMENT ON and SECURITY LABEL changes are good
> independently as well, but there previously have been raised some
> concerns about shared objects. I think the answer in the patches which
> is to raise events when affecting database local objects makes sense,
> but others might disagree.

Yes, I will push these unless somebody objects soon, as they seem
perfectly reasonable to me. The only troubling thing is that there is
no regression test for this kind of thing in event triggers (i.e. verify
which command tags get support and which don't), which seems odd to me.
Not these patches's fault, though, so I'm not considering adding any ATM.

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