Re: deparsing utility commands - Mailing list pgsql-hackers

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Andres Freund wrote:
> > On 2015-02-15 01:48:15 -0300, Alvaro Herrera wrote:
> > 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.

That looked fine to me.

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

It's not a huge complaint, but it feels a bit awkward to me that
ExecRenameStmt is now returning one item and using an out variable for
the other when the two really go together (Oid and Object Sub ID, that
is).  Further, the comment above ExecRenameStmt should make it clear
that it's safe to pass NULL into objsubid if you don't care about it.

The same probably goes for the COMMENT bits.

> 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 agree- I'm pretty sure we definitely don't want to run through
transformExpr again in the deparse code.  I'm not a huge fan of adding a
Node* output parameter, but I havn't got any other great ideas about how
to address that.

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

Ugh.  I dislike that when we say an event trigger will fire before
'GRANT' what we really mean is "GRANT when it's operating against a
local object".  At the minimum we absolutely need to be very clear in
the documentation about that limitation.  Perhaps there is something
already which reflects that, but I don't see anything in the patch
being added about that.

Still looking at the rest of the patches.
Thanks!
    Stephen