Re: deparsing utility commands

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: deparsing utility commands
Date: 2015-03-16 23:44:06
Message-ID: 20150316234406.GH3636@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-add-secondaryOid-to-DefineTSConfiguration.patch text/x-diff 2.1 KB
0002-deparse-core-have-ALTER-TABLE-return-table-OID-and-o.patch text/x-diff 41.6 KB
0003-deparse-infrastructure-needed-for-command-deparsing.patch text/x-diff 69.5 KB
0004-deparse-add-EventTriggerStashCommand-calls-to-Proces.patch text/x-diff 18.2 KB
0005-deparse-Support-CREATE-SCHEMA-TABLE-SEQUENCE-INDEX-T.patch text/x-diff 54.0 KB
0006-deparse-Support-CREATE-TYPE-AS-ENUM-RANGE.patch text/x-diff 5.0 KB
0007-deparse-Support-EXTENSION-commands.patch text/x-diff 5.7 KB
0008-deparse-Support-CREATE-RULE.patch text/x-diff 6.4 KB
0009-deparse-Support-ALTER-TYPE-ADD-VALUE-enums.patch text/x-diff 2.0 KB
0010-deparse-Support-ALTER-.-RENAME-OWNER-SCHEMA.patch text/x-diff 22.4 KB
0011-deparse-Support-CREATE-ALTER-DOMAIN.patch text/x-diff 6.8 KB
0012-deparse-Support-CREATE-ALTER-FUNCTION.patch text/x-diff 16.1 KB
0013-deparse-Support-ALTER-TABLE.patch text/x-diff 32.5 KB
0014-deparse-Support-CREATE-VIEW.patch text/x-diff 3.2 KB
0015-deparse-Support-CREATE-OPERATOR-FAMILY.patch text/x-diff 2.1 KB
0016-deparse-Support-CREATE-CONVERSION.patch text/x-diff 2.2 KB
0017-deparse-Support-DefineStmt-commands.patch text/x-diff 25.4 KB
0018-deparse-support-CREATE-CAST.patch text/x-diff 3.2 KB
0019-deparse-Support-GRANT-REVOKE.patch text/x-diff 15.0 KB
0020-deparse-support-COMMENT-ON-and-SECURITY-LABEL.patch text/x-diff 5.7 KB
0021-deparse-Handle-default-security-provider.patch text/x-diff 960 bytes
0022-deparse-support-CREATE-TABLE-AS.patch text/x-diff 6.3 KB
0023-deparse-support-CREATE-ALTER-POLICY.patch text/x-diff 5.0 KB
0024-deparse-support-REFRESH-MATERIALIZED-VIEW.patch text/x-diff 1.8 KB
0025-deparse-support-FDW-related-commands.patch text/x-diff 14.4 KB
0026-infrastructure-for-ALTER-OPERATOR-FAMILY.patch text/x-diff 6.9 KB
0027-deparse-Support-ALTER-OPERATOR-FAMILY.patch text/x-diff 8.8 KB
0028-deparse-support-ALTER-DEFAULT-PRIVILEGES.patch text/x-diff 8.8 KB
0029-deparse-support-CREATE-OPERATOR-CLASS.patch text/x-diff 11.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2015-03-17 01:09:19 Re: ranlib bleating about dirmod.o being empty
Previous Message Dean Rasheed 2015-03-16 21:05:54 Re: get_object_address support for additional object types