deparsing utility commands

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

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) 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've started taking a look at this as the pgaudit bits depend on it and
noticed that the patch set implies there's 42 patches, but there were
only 37 attached..?

Thanks!

Stephen

Stephen Frost wrote:

I've started taking a look at this as the pgaudit bits depend on it and
noticed that the patch set implies there's 42 patches, but there were
only 37 attached..?

Ah, I didn't realize when I posted that the subject was counting those
extra patches. They are later patches that add the testing framework,
but since the tests don't pass currently, they are not usable yet.
Mostly they are about the running deparse_init.sql file that I posted
separately. I will post a real patch for that stuff later today to make
it clear what it is that we're talking about.

FWIW, one of Robert's main objections is that future hackers will forget
to add deparse support for new commands as they are added. In an
attempt to get this sorted out, I have modified the stuff in
ProcessUtilitySlow() so that instead of having one
EventTriggerStashCommand call for each node type, there is only one call
at the end of the function. That way, new cases in the big switch there
will automatically get something added to the stash, which should make
the test fail appropriately. (Things like adding a new clause to
existing commands will be tested by running pg_dump on the databases and
comparing.)

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Stephen Frost wrote:

I've started taking a look at this as the pgaudit bits depend on it and
noticed that the patch set implies there's 42 patches, but there were
only 37 attached..?

Ah, I didn't realize when I posted that the subject was counting those
extra patches. They are later patches that add the testing framework,
but since the tests don't pass currently, they are not usable yet.
Mostly they are about the running deparse_init.sql file that I posted
separately. I will post a real patch for that stuff later today to make
it clear what it is that we're talking about.

Oh, ok, no problem, just wanted to make sure things weren't missing.

FWIW, one of Robert's main objections is that future hackers will forget
to add deparse support for new commands as they are added. In an
attempt to get this sorted out, I have modified the stuff in
ProcessUtilitySlow() so that instead of having one
EventTriggerStashCommand call for each node type, there is only one call
at the end of the function. That way, new cases in the big switch there
will automatically get something added to the stash, which should make
the test fail appropriately. (Things like adding a new clause to
existing commands will be tested by running pg_dump on the databases and
comparing.)

Right, I've been following the discussion and fully agree with Robert
that we really need a way to make sure that future work doesn't forget
to address deparseing (or the various other bits, object classes, etc,
really). The approach you've outlined sounds interesting but I haven't
yet gotten to that bit of the code.

Thanks!

Stephen

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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

Stephen,

Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

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.

Hmm, while I agree that it's a relatively minor point, it seems a fair
one. I think we could handle this by returning ObjectAddress rather
than Oid in ExecRenameStmt() and CommentObject(); then you have all the
bits you need in a single place. Furthermore, the function in another
patch EventTriggerStashCommand() instead of getting separately (ObjType,
objectId, objectSubId) could take a single argument of type
ObjectAddress.

Now, we probably don't want to hack *all* the utility commands to return
ObjectAddress instead of OID, because it many cases that's just not
going to be convenient (not to speak of the code churn); so I think for
most objtypes the ProcessUtilitySlow stanza would look like this:

case T_AlterTSConfigurationStmt:
objectId = AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree);
objectType = OBJECT_TSCONFIGURATION;
break;

For ExecRenameStmt and CommentObject (and probably other cases such as
security labels) the stanza in ProcessUtilitySlow would be simpler:

case T_CommentStmt:
address = CommentObject((CommentStmt *) parsetree);
break;

and at the bottom of the loop we would transform the objid/type into
address for the cases that need it:

if (!commandStashed)
{
if (objectId != InvalidOid)
{
address.classId = get_objtype_catalog_oid(objectType);
address.objectId = objectId;
address.objectSubId = 0;
}
EventTriggerStashCommand(address, secondaryOid, parsetree);
}

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.

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.

Yeah, my thoughts exactly :-(

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.

Hmm, good point, will give this some thought. I'm thinking perhaps we
can add a table of which object types are supported for generic commands
such as GRANT, COMMENT and SECURITY LABEL.

Still looking at the rest of the patches.

Great, thanks -- and thanks for the review so far.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

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.

Hmm, while I agree that it's a relatively minor point, it seems a fair
one. I think we could handle this by returning ObjectAddress rather
than Oid in ExecRenameStmt() and CommentObject(); then you have all the
bits you need in a single place. Furthermore, the function in another
patch EventTriggerStashCommand() instead of getting separately (ObjType,
objectId, objectSubId) could take a single argument of type
ObjectAddress.

Agreed, that thought occured to me as well while I was looking through
the other deparse patches and I agree that it makes sense.

Now, we probably don't want to hack *all* the utility commands to return
ObjectAddress instead of OID, because it many cases that's just not
going to be convenient (not to speak of the code churn); so I think for
most objtypes the ProcessUtilitySlow stanza would look like this:

case T_AlterTSConfigurationStmt:
objectId = AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree);
objectType = OBJECT_TSCONFIGURATION;
break;

For ExecRenameStmt and CommentObject (and probably other cases such as
security labels) the stanza in ProcessUtilitySlow would be simpler:

case T_CommentStmt:
address = CommentObject((CommentStmt *) parsetree);
break;

and at the bottom of the loop we would transform the objid/type into
address for the cases that need it:

if (!commandStashed)
{
if (objectId != InvalidOid)
{
address.classId = get_objtype_catalog_oid(objectType);
address.objectId = objectId;
address.objectSubId = 0;
}
EventTriggerStashCommand(address, secondaryOid, parsetree);
}

That'd be fine with me, though for my 2c, I wouldn't object to changing
them all to return ObjectAddress either. I agree that it'd cause a fair
bit of code churn to do so, but there's a fair bit of code churn
happening here anyway (looking at what 0008 does to ProcessUtilitySlow
anyway).

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.

Hmm, good point, will give this some thought. I'm thinking perhaps we
can add a table of which object types are supported for generic commands
such as GRANT, COMMENT and SECURITY LABEL.

That sounds like a good idea.

Thanks!

Stephen

Now, we probably don't want to hack *all* the utility commands to return
ObjectAddress instead of OID, because it many cases that's just not
going to be convenient (not to speak of the code churn); so I think for
most objtypes the ProcessUtilitySlow stanza would look like this:

That'd be fine with me, though for my 2c, I wouldn't object to changing
them all to return ObjectAddress either. I agree that it'd cause a fair
bit of code churn to do so, but there's a fair bit of code churn
happening here anyway (looking at what 0008 does to ProcessUtilitySlow
anyway).

Well, that would make my life easier I think (even if it's a bit more
work), so unless there are objections I will do things this way. It's a
bit of a pity that Robert and Dimitri went to huge lengths to have these
functions return OID and we're now changing it all to ObjAddress
instead, but oh well.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Now, we probably don't want to hack *all* the utility commands to return
ObjectAddress instead of OID, because it many cases that's just not
going to be convenient (not to speak of the code churn); so I think for
most objtypes the ProcessUtilitySlow stanza would look like this:

That'd be fine with me, though for my 2c, I wouldn't object to changing
them all to return ObjectAddress either. I agree that it'd cause a fair
bit of code churn to do so, but there's a fair bit of code churn
happening here anyway (looking at what 0008 does to ProcessUtilitySlow
anyway).

Well, that would make my life easier I think (even if it's a bit more
work), so unless there are objections I will do things this way. It's a
bit of a pity that Robert and Dimitri went to huge lengths to have these
functions return OID and we're now changing it all to ObjAddress
instead, but oh well.

Not sure that I see it as that huge a deal.. They're still returning an
Oid, it's just embedded in the ObjAddress to provide a complete
statement of what the object is.

btw, the hunk in 0026 which adds a 'break;' into standard_ProcessUtility
caught me by surprise. Looks like that 'break;' was missing from 0003
(for T_GrantStmt).

Thanks,

Stephen

Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Now, we probably don't want to hack *all* the utility commands to return
ObjectAddress instead of OID, because it many cases that's just not
going to be convenient (not to speak of the code churn); so I think for
most objtypes the ProcessUtilitySlow stanza would look like this:

That'd be fine with me, though for my 2c, I wouldn't object to changing
them all to return ObjectAddress either. I agree that it'd cause a fair
bit of code churn to do so, but there's a fair bit of code churn
happening here anyway (looking at what 0008 does to ProcessUtilitySlow
anyway).

Well, that would make my life easier I think (even if it's a bit more
work), so unless there are objections I will do things this way. It's a
bit of a pity that Robert and Dimitri went to huge lengths to have these
functions return OID and we're now changing it all to ObjAddress
instead, but oh well.

Not sure that I see it as that huge a deal.. They're still returning an
Oid, it's just embedded in the ObjAddress to provide a complete
statement of what the object is.

I've been looking at this idea some more. There's some churn, but it's
not that bad. For instance, here's the patch to have RENAME return
ObjectAddress rather than OIDs.

For instance, the get_objtype_catalog_id() function, provided in a later
patch in the series, will no longer be necessary; instead, each
execution code path in the src/backend/command code must set the correct
catalog ID in the ObjectAddress it returns. So the event triggers code
will have the catalog ID at the ready, without having to figure it out
from the ObjectType it gets from the parse node.

btw, the hunk in 0026 which adds a 'break;' into standard_ProcessUtility
caught me by surprise. Looks like that 'break;' was missing from 0003
(for T_GrantStmt).

Thanks for pointing this out -- that was broken rebase.

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

Stephen Frost wrote:

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.

Hmm, good point, will give this some thought. I'm thinking perhaps we
can add a table of which object types are supported for generic commands
such as GRANT, COMMENT and SECURITY LABEL.

That sounds like a good idea.

Here's a patch. I noticed that the introduction to event trigger
already says they only act on local objects:

The ddl_command_start event occurs just before the execution of
a CREATE, ALTER, DROP, SECURITY LABEL, COMMENT, GRANT or REVOKE
command. No check whether the affected object exists or doesn't
exist is performed before the event trigger fires. As an
exception, however, this event does not occur for DDL commands
targeting shared objects — databases, roles, and tablespaces —
or for commands targeting event triggers themselves.

So adding more text to the same effect would be repetitive. I added a
sixth column "Notes" to the table of supported command tags vs. events,
with the text "Only for local objects" next to the four commands being
added here.

I think it's fair to push this patch as is.

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

On 2015-02-19 17:14:57 -0300, Alvaro Herrera wrote:

The ddl_command_start event occurs just before the execution of
a CREATE, ALTER, DROP, SECURITY LABEL, COMMENT, GRANT or REVOKE
command. No check whether the affected object exists or doesn't
exist is performed before the event trigger fires. As an
exception, however, this event does not occur for DDL commands
targeting shared objects — databases, roles, and tablespaces —
or for commands targeting event triggers themselves.

So adding more text to the same effect would be repetitive. I added a
sixth column "Notes" to the table of supported command tags vs. events,
with the text "Only for local objects" next to the four commands being
added here.

I think that's sufficient for now. We might want to expand the handling
of 'global objects' topic sometime in the future, but for now I think
what you have is clear enough.

I think it's fair to push this patch as is.

+1

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2015-02-19 14:39:27 -0300, Alvaro Herrera wrote:

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d899dd7..2bbc15d 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -531,6 +531,12 @@ ObjectTypeMap[] =
{ "policy", OBJECT_POLICY }
};

+ObjectAddress InvalidObjectAddress =
+{
+	InvalidOid,
+	InvalidOid,
+	0
+};

Maybe mark it as a constant? Then it can live in some readonly section.

+extern ObjectAddress InvalidObjectAddress;
+
+#define initObjectAddress(addr, class_id, object_id) \
+	do { \
+		(addr).classId = (class_id); \
+		(addr).objectId = (object_id); \
+		(addr).objectSubId = 0; \
+	} while (0)
+
+#define initObjectAddressSub(addr, class_id, object_id, object_sub_id) \
+	do { \
+		(addr).classId = (class_id); \
+		(addr).objectId = (object_id); \
+		(addr).objectSubId = (object_sub_id); \
+	} while (0)
+

Maybe, based on some precedent, make those ObjectAddressSet(Sub)?() -
the init bit in initObjectAddress sounds to me like like it does more
than setting a couple member variables.

I'd also be inclined to make initObjectAddress use initObjectAddressSub,
but that's reaching the realm of pedantism ;)

To me the change sounds like a good idea - drop/rename are already
handled somewhat generically, and expanding that to the return value for
renames sounds like a natural progression to me.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2015-02-15 01:48:15 -0300, Alvaro Herrera wrote:

One line of defense which I just tought about is that instead of
sprinkling EventTriggerStashCommand() calls all over ProcessUtilitySlow,
we should add only one at the bottom.

Doesn't sound like a bad idea, but I'm not sure whether it's realistic
given that some commands do multiple EventTriggerStashCommand()s?

1. ruleutils.c seems to deparse temp schemas as pg_temp_X instead of
just pg_temp; so some commands like CREATE VIEW..AS SELECT FROM temp_table
fail in an ugly way. Maybe the solution is to tell ruleutils that the
temp schema is always visible; or maybe the solution is to tell it that
it's spelled pg_temp instead.

Hm. I'm not immediately following why that's happening for deparsing but
not for the normal get_viewdef stuff?

From d03a8edcefd8f0ea1c46b315c446f96c61146a85 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Wed, 24 Sep 2014 15:53:04 -0300
Subject: [PATCH 07/42] deparse: infrastructure needed for command deparsing

This patch introduces unused infrastructure for command deparsing.
There are three main parts:

+ "as of yet"?

1. A list (stash) of executed commands in Node format, stored in the
event trigger state. At ddl_command_end, the stored items can be
extracted. For now this only support "basic" commands (in particular
not ALTER TABLE or GRANT). It's useful enough to cover all CREATE
command as well as many simple ALTER forms.

seems outdated.

(yea, I know you want to fold the patch anyway).

+/* XXX merge this with ObjectTypeMap? */
static event_trigger_support_data event_trigger_support[] = {
{"AGGREGATE", true},
{"CAST", true},

Hm, I'd just drop the XXX for now.

@@ -1060,6 +1066,7 @@ EventTriggerSupportsObjectType(ObjectType obtype)
case OBJECT_CAST:
case OBJECT_COLUMN:
case OBJECT_COLLATION:
+ case OBJECT_COMPOSITE:
case OBJECT_CONVERSION:
case OBJECT_DEFAULT:
case OBJECT_DOMAIN:
@@ -1088,6 +1095,7 @@ EventTriggerSupportsObjectType(ObjectType obtype)
case OBJECT_TSPARSER:
case OBJECT_TSTEMPLATE:
case OBJECT_TYPE:
+ case OBJECT_USER_MAPPING:
case OBJECT_VIEW:
return true;
}

Should those two be broken out and just committed?

@@ -1193,14 +1201,6 @@ EventTriggerBeginCompleteQuery(void)
EventTriggerQueryState *state;
MemoryContext cxt;

-	/*
-	 * Currently, sql_drop and table_rewrite events are the only reason to
-	 * have event trigger state at all; so if there are none, don't install
-	 * one.
-	 */
-	if (!trackDroppedObjectsNeeded())
-		return false;
-
cxt = AllocSetContextCreate(TopMemoryContext,
"event trigger state",
ALLOCSET_DEFAULT_MINSIZE,
@@ -1211,7 +1211,7 @@ EventTriggerBeginCompleteQuery(void)
slist_init(&(state->SQLDropList));
state->in_sql_drop = false;
state->table_rewrite_oid = InvalidOid;
-
+	state->stash = NIL;
state->previous = currentEventTriggerState;
currentEventTriggerState = state;

Hm. I'm not clear why we don't check for other types of event triggers
in EventTriggerBeginCompleteQuery and skip the work otherwise. If we
just enable them all the time - which imo is ok given how they're split
of in the normal process utility - we should remove the boolean return
value from Begin/CompleteQuery and drop
* Note: it's an error to call this routine if EventTriggerBeginCompleteQuery
* returned false previously.
from EndCompleteQuery.

+/*
+ * EventTriggerStashCommand
+ * 		Save data about a simple DDL command that was just executed
+ */
+void
+EventTriggerStashCommand(Oid objectId, uint32 objectSubId, ObjectType objtype,
+						 Oid secondaryOid, Node *parsetree)
+{
+	MemoryContext oldcxt;
+	StashedCommand *stashed;
+
+	oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
+
+	stashed = palloc(sizeof(StashedCommand));
+
+	stashed->type = SCT_Simple;
+	stashed->in_extension = creating_extension;
+
+	stashed->d.simple.objectId = objectId;
+	stashed->d.simple.objtype = objtype;
+	stashed->d.simple.objectSubId = objectSubId;
+	stashed->d.simple.secondaryOid = secondaryOid;
+	stashed->parsetree = copyObject(parsetree);
+
+	currentEventTriggerState->stash = lappend(currentEventTriggerState->stash,
+											  stashed);
+
+	MemoryContextSwitchTo(oldcxt);
+}

It's a bit wierd that the drop list is managed using a slist, but the
stash isn't...

+Datum
+pg_event_trigger_get_creation_commands(PG_FUNCTION_ARGS)
+{

+	foreach(lc, currentEventTriggerState->stash)
+	{

+		command = deparse_utility_command(cmd);
+
+		/*
+		 * Some parse trees return NULL when deparse is attempted; we don't
+		 * emit anything for them.
+		 */
+		if (command != NULL)
+		{
+			Datum		values[9];
+			bool		nulls[9];
+			ObjectAddress addr;
+			int			i = 0;
+
+			MemSet(nulls, 0, sizeof(nulls));
+
+			if (cmd->type == SCT_Simple)
+			{
+				Oid			classId;
+				Oid			objId;
+				uint32		objSubId;
+				const char *tag;
+				char	   *identity;
+				char	   *type;
+				char	   *schema = NULL;
+
+				if (cmd->type == SCT_Simple)
+				{
+					classId = get_objtype_catalog_oid(cmd->d.simple.objtype);
+					objId = cmd->d.simple.objectId;
+					objSubId = cmd->d.simple.objectSubId;
+				}
+
+				tag = CreateCommandTag(cmd->parsetree);
+				addr.classId = classId;
+				addr.objectId = objId;
+				addr.objectSubId = objSubId;
+
+				type = getObjectTypeDescription(&addr);
+				identity = getObjectIdentity(&addr);
+
+				/*
+				 * Obtain schema name, if any ("pg_temp" if a temp object)
+				 */
+				if (is_objectclass_supported(addr.classId))
+				{
+					AttrNumber	nspAttnum;
+
+					nspAttnum = get_object_attnum_namespace(addr.classId);
+					if (nspAttnum != InvalidAttrNumber)
+					{
+						Relation	catalog;
+						HeapTuple	objtup;
+						Oid			schema_oid;
+						bool		isnull;
+
+						catalog = heap_open(addr.classId, AccessShareLock);
+						objtup = get_catalog_object_by_oid(catalog,
+														   addr.objectId);
+						if (!HeapTupleIsValid(objtup))
+							elog(ERROR, "cache lookup failed for object %u/%u",
+								 addr.classId, addr.objectId);
+						schema_oid = heap_getattr(objtup, nspAttnum,
+												  RelationGetDescr(catalog), &isnull);
+						if (isnull)
+							elog(ERROR, "invalid null namespace in object %u/%u/%d",
+								 addr.classId, addr.objectId, addr.objectSubId);
+						if (isAnyTempNamespace(schema_oid))
+							schema = pstrdup("pg_temp");
+						else
+							schema = get_namespace_name(schema_oid);
+
+						heap_close(catalog, AccessShareLock);
+					}
+				}

Hm. How do we get here for !is_objectclass_supported()?

+/*
+ * Allocate a new object tree to store parameter values -- varargs version.
+ *
+ * The "fmt" argument is used to append as a "fmt" element in the output blob.
+ * numobjs indicates the number of extra elements to append; for each one, a
+ * name (string), type (from the ObjType enum) and value must be supplied.  The
+ * value must match the type given; for instance, ObjTypeInteger requires an
+ * int64, ObjTypeString requires a char *, ObjTypeArray requires a list (of
+ * ObjElem), ObjTypeObject requires an ObjTree, and so on.  Each element type *
+ * must match the conversion specifier given in the format string, as described
+ * in pg_event_trigger_expand_command, q.v.
+ *
+ * Note we don't have the luxury of sprintf-like compiler warnings for
+ * malformed argument lists.
+ */
+static ObjTree *
+new_objtree_VA(char *fmt, int numobjs,...)
+{
+	ObjTree    *tree;
+	va_list		args;
+	int			i;
+
+	/* Set up the toplevel object and its "fmt" */
+	tree = new_objtree();
+	append_string_object(tree, "fmt", fmt);
+
+	/* And process the given varargs */
+	va_start(args, numobjs);
+	for (i = 0; i < numobjs; i++)
+	{
+		ObjTree    *value;
+		ObjType		type;
+		ObjElem	   *elem;
+		char	   *name;
+		char	   *strval;
+		bool		boolval;
+		List	   *list;
+		int64		number;
+		float8		fnumber;
+
+		name = va_arg(args, char *);
+		type = va_arg(args, ObjType);
+
+		/* Null params don't have a value (obviously) */
+		if (type == ObjTypeNull)
+		{
+			append_null_object(tree, name);
+			continue;
+		}
+
+		/*
+		 * For all other param types there must be a value in the varargs.
+		 * Fetch it and add the fully formed subobject into the main object.
+		 */
+		switch (type)
+		{
+			case ObjTypeBool:
+				boolval = va_arg(args, int);
+				elem = new_bool_object(boolval);
+				break;
+			case ObjTypeString:
+				strval = va_arg(args, char *);
+				elem = new_string_object(strval);
+				break;
+			case ObjTypeObject:
+				value = va_arg(args, ObjTree *);
+				elem = new_object_object(value);
+				break;
+			case ObjTypeArray:
+				list = va_arg(args, List *);
+				elem = new_array_object(list);
+				break;
+			case ObjTypeInteger:
+				number = va_arg(args, int64);
+				elem = new_integer_object(number);
+				break;
+			case ObjTypeFloat:
+				fnumber = va_arg(args, double);
+				elem = new_float_object(fnumber);
+				break;
+			default:
+				elog(ERROR, "invalid parameter type %d", type);
+		}
+
+		elem->name = name;
+		append_premade_object(tree, elem);
+	}
+
+	va_end(args);
+	return tree;
+}

Would look nicer if boolval et al weren't declared - I'd just pass the
va_arg() return value directly to new_*_object().

Attached as 0001.

Additionally I find the separate handling of ObjTypeNull not so
nice. 0002.

+/*
+ * A helper routine to setup %{}T elements.
+ */
+static ObjTree *
+new_objtree_for_type(Oid typeId, int32 typmod)
+{

+ append_bool_object(typeParam, "is_array", is_array);

Maybe name that one typarray?

+/*
+ * Handle deparsing of simple commands.
+ *
+ * This function contains a large switch that mirrors that in
+ * ProcessUtilitySlow.  All cases covered there should also be covered here.
+ */
+static ObjTree *
+deparse_simple_command(StashedCommand *cmd)
+{
+
+		case T_CommentStmt:
+			command = NULL;
+			break;
+
+		case T_GrantStmt:
+			/* handled elsewhere */
+			elog(ERROR, "unexpected command type %s", CreateCommandTag(parsetree));
+			break;

...

+		case T_SecLabelStmt:
+			elog(ERROR, "unimplemented deparse of %s", CreateCommandTag(parsetree));
+			break;
+	}
+
+	return command;
+}

It is not clear to me why some commands error out and other don't. It
makes sense to me to error out for things like GrantStmt that shouldn't
end up here, but... I guess that's just an artifact of the patch series
because you add the handling for the non elog()ing commands later?

I think this should use ereport() in some cases if we're not going to
support some commands for now.

+/*
+ * Given a utility command parsetree and the OID of the corresponding object,
+ * return a JSON representation of the command.
+ *
+ * The command is expanded fully, so that there are no ambiguities even in the
+ * face of search_path changes.
+ */
+char *
+deparse_utility_command(StashedCommand *cmd)
+{
+	OverrideSearchPath *overridePath;
+	MemoryContext	oldcxt;
+	MemoryContext	tmpcxt;
+	ObjTree		   *tree;
+	char		   *command;
+	StringInfoData  str;
+
+	/*
+	 * Allocate everything done by the deparsing routines into a temp context,
+	 * to avoid having to sprinkle them with memory handling code; but allocate
+	 * the output StringInfo before switching.
+	 */
+	initStringInfo(&str);
+	tmpcxt = AllocSetContextCreate(CurrentMemoryContext,
+								   "deparse ctx",
+								   ALLOCSET_DEFAULT_MINSIZE,
+								   ALLOCSET_DEFAULT_INITSIZE,
+								   ALLOCSET_DEFAULT_MAXSIZE);
+	oldcxt = MemoryContextSwitchTo(tmpcxt);
+
+	/*
+	 * Many routines underlying this one will invoke ruleutils.c functionality
+	 * in order to obtain deparsed versions of expressions.  In such results,
+	 * we want all object names to be qualified, so that results are "portable"
+	 * to environments with different search_path settings.  Rather than inject
+	 * what would be repetitive calls to override search path all over the
+	 * place, we do it centrally here.
+	 */
+	overridePath = GetOverrideSearchPath(CurrentMemoryContext);
+	overridePath->schemas = NIL;
+	overridePath->addCatalog = false;
+	overridePath->addTemp = false;
+	PushOverrideSearchPath(overridePath);

Ah, the 'addTemp = false' probably is the answer to my question above
about view deparsing adding a fully qualified pg_temp...

+/*------
+ * Returns a formatted string from a JSON object.
+ *
+ * The starting point is the element named "fmt" (which must be a string).
+ * This format string may contain zero or more %-escapes, which consist of an
+ * element name enclosed in { }, possibly followed by a conversion modifier,
+ * followed by a conversion specifier.	Possible conversion specifiers are:
+ *
+ * %		expand to a literal %.
+ * I		expand as a single, non-qualified identifier
+ * D		expand as a possibly-qualified identifier
+ * T		expand as a type name
+ * O		expand as an operator name
+ * L		expand as a string literal (quote using single quotes)
+ * s		expand as a simple string (no quoting)
+ * n		expand as a simple number (no quoting)
+ *
+ * The element name may have an optional separator specification preceded
+ * by a colon.	Its presence indicates that the element is expected to be
+ * an array; the specified separator is used to join the array elements.
+ *------

I think this documentation should at least be referred to from
comments in deparse_utility.c. I was wondering where it is.

diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 6fbe283..a842ef2 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -416,7 +416,7 @@ JsonbToCString(StringInfo out, JsonbContainer *in, int estimated_len)
{
bool		first = true;
JsonbIterator *it;
-	int			type = 0;
+	JsonbIteratorToken type = WJB_DONE;
JsonbValue	v;
int			level = 0;
bool		redo_switch = false;
@@ -498,7 +498,7 @@ JsonbToCString(StringInfo out, JsonbContainer *in, int estimated_len)
first = false;
break;
default:
-				elog(ERROR, "unknown flag of jsonb iterator");
+				elog(ERROR, "unknown jsonb iterator token type");
}
}

Huh?

I think this infrastructure pretty desperately requires some higher
level overview. Like how are we going from the node tree, to the object
tree, to jsonb, to actual DDL. I think I understand, but it took me a
while. Doesn't have to be long and super detailed...

From dcb353c8c9bd93778a62719ff8bf32b9a419e16d Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org> Date: Thu, 25 Sep 2014
15:54:00 -0300 Subject: [PATCH 09/42] deparse: Support CREATE TYPE AS

+static ObjTree *
+deparse_ColumnDef(Relation relation, List *dpcontext, bool composite,
+				  ColumnDef *coldef)
+{

+	if (!composite)
+	{
+		/*
+		 * Emit a NOT NULL declaration if necessary.  Note that we cannot trust
+		 * pg_attribute.attnotnull here, because that bit is also set when
+		 * primary keys are specified; and we must not emit a NOT NULL
+		 * constraint in that case, unless explicitely specified.  Therefore,
+		 * we scan the list of constraints attached to this column to determine
+		 * whether we need to emit anything.
+		 * (Fortunately, NOT NULL constraints cannot be table constraints.)
+		 */

Is the primary key bit really a problem? To me it sounds like it's
actually good to retain a NOT NULL in that case. Note that pg_dump
actually *does* emit a NOT NULL here, even if you drop the primary
key. Since the column behaves as NOT NULL and is dumped as such it seems
like a good idea to fully treat it as such.

deparse: Support CREATE SCHEMA/TABLE/SEQUENCE/INDEX/TRIGGER

+/*
+ * deparse_CreateTrigStmt
+ *		Deparse a CreateTrigStmt (CREATE TRIGGER)
+ *
+ * Given a trigger OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */
+static ObjTree *
+deparse_CreateTrigStmt(Oid objectId, Node *parsetree)
+{
+	CreateTrigStmt *node = (CreateTrigStmt *) parsetree;
+	Relation	pg_trigger;
+	HeapTuple	trigTup;
+	Form_pg_trigger trigForm;
+	ObjTree	   *trigger;
+	ObjTree	   *tmp;
+	int			tgnargs;
+	List	   *list;
+	List	   *events;
+
+	pg_trigger = heap_open(TriggerRelationId, AccessShareLock);
+
+	trigTup = get_catalog_object_by_oid(pg_trigger, objectId);
+	trigForm = (Form_pg_trigger) GETSTRUCT(trigTup);
+
+	/*
+	 * Some of the elements only make sense for CONSTRAINT TRIGGERs, but it
+	 * seems simpler to use a single fmt string for both kinds of triggers.
+	 */
+	trigger =
+		new_objtree_VA("CREATE %{constraint}s TRIGGER %{name}I %{time}s %{events: OR }s "
+					   "ON %{relation}D %{from_table}s %{constraint_attrs: }s "
+					   "FOR EACH %{for_each}s %{when}s EXECUTE PROCEDURE %{function}s",
+					   2,
+					   "name", ObjTypeString, node->trigname,
+					   "constraint", ObjTypeString,
+					   node->isconstraint ? "CONSTRAINT" : "");
+
+	if (node->timing == TRIGGER_TYPE_BEFORE)
+		append_string_object(trigger, "time", "BEFORE");
+	else if (node->timing == TRIGGER_TYPE_AFTER)
+		append_string_object(trigger, "time", "AFTER");
+	else if (node->timing == TRIGGER_TYPE_INSTEAD)
+		append_string_object(trigger, "time", "INSTEAD OF");
+	else
+		elog(ERROR, "unrecognized trigger timing value %d", node->timing);
+
+	/*
+	 * Decode the events that the trigger fires for.  The output is a list;
+	 * in most cases it will just be a string with the even name, but when
+	 * there's an UPDATE with a list of columns, we return a JSON object.
+	 */
+	events = NIL;
+	if (node->events & TRIGGER_TYPE_INSERT)
+		events = lappend(events, new_string_object("INSERT"));
+	if (node->events & TRIGGER_TYPE_DELETE)
+		events = lappend(events, new_string_object("DELETE"));
+	if (node->events & TRIGGER_TYPE_TRUNCATE)
+		events = lappend(events, new_string_object("TRUNCATE"));
+	if (node->events & TRIGGER_TYPE_UPDATE)
+	{
+		if (node->columns == NIL)
+		{
+			events = lappend(events, new_string_object("UPDATE"));
+		}
+		else
+		{
+			ObjTree	   *update;
+			ListCell   *cell;
+			List	   *cols = NIL;
+
+			/*
+			 * Currently only UPDATE OF can be objects in the output JSON, but
+			 * we add a "kind" element so that user code can distinguish
+			 * possible future new event types.
+			 */
+			update = new_objtree_VA("UPDATE OF %{columns:, }I",
+									1, "kind", ObjTypeString, "update_of");
+
+			foreach(cell, node->columns)
+			{
+				char   *colname = strVal(lfirst(cell));
+
+				cols = lappend(cols,
+							   new_string_object(colname));
+			}
+
+			append_array_object(update, "columns", cols);
+
+			events = lappend(events,
+							 new_object_object(update));
+		}
+	}
+	append_array_object(trigger, "events", events);
+
+	tmp = new_objtree_for_qualname_id(RelationRelationId,
+									  trigForm->tgrelid);
+	append_object_object(trigger, "relation", tmp);
+
+	tmp = new_objtree_VA("FROM %{relation}D", 0);
+	if (trigForm->tgconstrrelid)
+	{
+		ObjTree	   *rel;
+
+		rel = new_objtree_for_qualname_id(RelationRelationId,
+										  trigForm->tgconstrrelid);
+		append_object_object(tmp, "relation", rel);
+	}
+	else
+		append_bool_object(tmp, "present", false);
+	append_object_object(trigger, "from_table", tmp);
+
+	list = NIL;
+	if (node->deferrable)
+		list = lappend(list,
+					   new_string_object("DEFERRABLE"));
+	if (node->initdeferred)
+		list = lappend(list,
+					   new_string_object("INITIALLY DEFERRED"));

I mildly wonder if DEFERRABLE/INITIALLY DEFERRED shouldn't instead have
an 'is_present' style representation.

+/*
+ * deparse_CreateStmt
+ *		Deparse a CreateStmt (CREATE TABLE)

I really wish CreateStmt were named CreateTableStmt. I don't know how
often that tripped me over, let alone everyone. Yes, that's an unrelated
rant ;)

+ * Given a table OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */
+static ObjTree *
+deparse_CreateStmt(Oid objectId, Node *parsetree)
+{

...

+	tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0);
+	if (node->tablespacename)
+		append_string_object(tmp, "tablespace", node->tablespacename);
+	else
+	{
+		append_null_object(tmp, "tablespace");
+		append_bool_object(tmp, "present", false);
+	}
+	append_object_object(createStmt, "tablespace", tmp);

Hm. I had not thought about that before, but given that
default_tablespace exists, we should maybe somehow indicate which one
was chosen?

+static ObjElem *
+deparse_Seq_OwnedBy(ObjTree *parent, Oid sequenceId)
+{
+	ObjTree    *ownedby = NULL;
+	Relation	depRel;
+	SysScanDesc scan;
+	ScanKeyData keys[3];
+	HeapTuple	tuple;
+
+	depRel = heap_open(DependRelationId, AccessShareLock);
+	ScanKeyInit(&keys[0],
+				Anum_pg_depend_classid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(RelationRelationId));
+	ScanKeyInit(&keys[1],
+				Anum_pg_depend_objid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(sequenceId));
+	ScanKeyInit(&keys[2],
+				Anum_pg_depend_objsubid,
+				BTEqualStrategyNumber, F_INT4EQ,
+				Int32GetDatum(0));
+
+	scan = systable_beginscan(depRel, DependDependerIndexId, true,
+							  NULL, 3, keys);
+	while (HeapTupleIsValid(tuple = systable_getnext(scan)))
+	{
+		Oid			ownerId;
+		Form_pg_depend depform;
+		ObjTree    *tmp;
+		char	   *colname;
+
+		depform = (Form_pg_depend) GETSTRUCT(tuple);
+
+		/* only consider AUTO dependencies on pg_class */
+		if (depform->deptype != DEPENDENCY_AUTO)
+			continue;
+		if (depform->refclassid != RelationRelationId)
+			continue;
+		if (depform->refobjsubid <= 0)
+			continue;
+
+		ownerId = depform->refobjid;
+		colname = get_attname(ownerId, depform->refobjsubid);
+		if (colname == NULL)
+			continue;
+
+		tmp = new_objtree_for_qualname_id(RelationRelationId, ownerId);
+		append_string_object(tmp, "attrname", colname);
+		ownedby = new_objtree_VA("OWNED BY %{owner}D",
+								 2,
+								 "clause", ObjTypeString, "owned",
+								 "owner", ObjTypeObject, tmp);
+	}
+
+	systable_endscan(scan);
+	relation_close(depRel, AccessShareLock);
+
+	/*
+	 * If there's no owner column, emit an empty OWNED BY element, set up so
+	 * that it won't print anything.
+	 */

"column"? Should have been row?

+	if (!ownedby)
+		/* XXX this shouldn't happen ... */
+		ownedby = new_objtree_VA("OWNED BY %{owner}D",
+								 3,
+								 "clause", ObjTypeString, "owned",
+								 "owner", ObjTypeNull,
+								 "present", ObjTypeBool, false);

Why shouldn't this happen? Free standing sequences are perfectly normal?
And OWNED BY NONE will need to be deparseable.

+static ObjTree *
+deparse_CreateSeqStmt(Oid objectId, Node *parsetree)
+{
+	ObjTree    *createSeq;
+	ObjTree    *tmp;
+	Relation	relation = relation_open(objectId, AccessShareLock);
+	Form_pg_sequence seqdata;
+	List	   *elems = NIL;
+
+	seqdata = get_sequence_values(objectId);
+
+	createSeq =
+		new_objtree_VA("CREATE %{persistence}s SEQUENCE %{identity}D "
+					   "%{definition: }s",
+					   1,
+					   "persistence", ObjTypeString,
+					   get_persistence_str(relation->rd_rel->relpersistence));
+
+	tmp = new_objtree_for_qualname(relation->rd_rel->relnamespace,
+								   RelationGetRelationName(relation));
+	append_object_object(createSeq, "identity", tmp);
+
+	/* definition elements */
+	elems = lappend(elems, deparse_Seq_Cache(createSeq, seqdata));
+	elems = lappend(elems, deparse_Seq_Cycle(createSeq, seqdata));
+	elems = lappend(elems, deparse_Seq_IncrementBy(createSeq, seqdata));
+	elems = lappend(elems, deparse_Seq_Minvalue(createSeq, seqdata));
+	elems = lappend(elems, deparse_Seq_Maxvalue(createSeq, seqdata));
+	elems = lappend(elems, deparse_Seq_Startwith(createSeq, seqdata));
+	elems = lappend(elems, deparse_Seq_Restart(createSeq, seqdata));
+	/* we purposefully do not emit OWNED BY here */

Needs explanation about the reason. Hm. I don't think it's actually
correct. Right now the following won't be deparsed correctly:

CREATE TABLE seqowner(id int);
CREATE SEQUENCE seqowned OWNED BY seqowner.id;
that generates
CREATE TABLE public.seqowner (id pg_catalog.int4 ) WITH (oids=OFF)
CREATE SEQUENCE public.seqowned CACHE 1 NO CYCLE INCREMENT BY 1 MINVALUE 1 MAXVALUE 9223372036854775807 START WITH 1 RESTART 1

+/*
+ * deparse_IndexStmt
+ *		deparse an IndexStmt
+ *
+ * Given an index OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ *
+ * If the index corresponds to a constraint, NULL is returned.
+ */
+static ObjTree *
+deparse_IndexStmt(Oid objectId, Node *parsetree)
+{

+	/* tablespace */
+	tmp = new_objtree_VA("TABLESPACE %{tablespace}s", 0);
+	if (tablespace)
+		append_string_object(tmp, "tablespace", tablespace);
+	else
+		append_bool_object(tmp, "present", false);
+	append_object_object(indexStmt, "tablespace", tmp);

Same default tablespace question as with create table...

From 4e35ba6557a6d04539d69d75821c568f9efb09b5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 14 Feb 2014 19:04:08 -0300
Subject: [PATCH 12/42] deparse: Support CREATE TYPE AS RANGE

+static ObjTree *
+deparse_CreateRangeStmt(Oid objectId, Node *parsetree)
+{

+	/* SUBTYPE_OPCLASS */
+	if (OidIsValid(rangeForm->rngsubopc))
+	{
+		tmp = new_objtree_for_qualname_id(OperatorClassRelationId,
+										  rangeForm->rngsubopc);
+		tmp = new_objtree_VA("SUBTYPE_OPCLASS = %{opclass}D",
+							 2,
+							 "clause", ObjTypeString, "opclass",
+							 "opclass", ObjTypeObject, tmp);
+		definition = lappend(definition, new_object_object(tmp));
+	}
+
+	/* COLLATION */
+	if (OidIsValid(rangeForm->rngcollation))
+	{
+		tmp = new_objtree_for_qualname_id(CollationRelationId,
+										  rangeForm->rngcollation);
+		tmp = new_objtree_VA("COLLATION = %{collation}D",
+							 2,
+							 "clause", ObjTypeString, "collation",
+							 "collation", ObjTypeObject, tmp);
+		definition = lappend(definition, new_object_object(tmp));
+	}
+
+	/* CANONICAL */
+	if (OidIsValid(rangeForm->rngcanonical))
+	{
+		tmp = new_objtree_for_qualname_id(ProcedureRelationId,
+										  rangeForm->rngcanonical);
+		tmp = new_objtree_VA("CANONICAL = %{canonical}D",
+							 2,
+							 "clause", ObjTypeString, "canonical",
+							 "canonical", ObjTypeObject, tmp);
+		definition = lappend(definition, new_object_object(tmp));
+	}
+
+	/* SUBTYPE_DIFF */
+	if (OidIsValid(rangeForm->rngsubdiff))
+	{
+		tmp = new_objtree_for_qualname_id(ProcedureRelationId,
+										  rangeForm->rngsubdiff);
+		tmp = new_objtree_VA("SUBTYPE_DIFF = %{subtype_diff}D",
+							 2,
+							 "clause", ObjTypeString, "subtype_diff",
+							 "subtype_diff", ObjTypeObject, tmp);
+		definition = lappend(definition, new_object_object(tmp));
+	}

Maybe use the present = false stuff here as well?

From 04dc40db971dc51c7e8c646cb5d822488da306f7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 21 Feb 2014 18:11:35 -0300
Subject: [PATCH 13/42] deparse: Support CREATE EXTENSION

/*
+ * deparse_CreateExtensionStmt
+ *		deparse a CreateExtensionStmt
+ *
+ * Given an extension OID and the parsetree that created it, return the JSON
+ * blob representing the creation command.
+ *
+ * XXX the current representation makes the output command dependant on the
+ * installed versions of the extension.  Is this a problem?

I tend to think it's ok. Might be worthwhile to add the final extension
version in some unused attribute?

From f45a9141905e93b83d28809e357f95c7fa713fe7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Wed, 26 Feb 2014 17:26:55 -0300
Subject: [PATCH 14/42] deparse: Support CREATE RULE

/me scheds some tears

From 67ff742875bfadd182ae28e40e54476c9e4d220e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 25 Apr 2014 16:43:53 -0300
Subject: [PATCH 16/42] deparse: Support for ALTER <OBJECT> RENAME

It supports everything but functions, aggregates, operator classes and
operator families.

AFAICS those are implemented, and you updated the description in git
since.

+static ObjTree *
+deparse_RenameStmt(Oid objectId, Node *parsetree)
+{
+	RenameStmt *node = (RenameStmt *) parsetree;
+	ObjTree	   *renameStmt;
+	char	   *fmtstr;
+	Relation	relation;
+	Oid			schemaId;
+
+	/*
+	 * FIXME --- this code is missing support for inheritance behavioral flags,
+	 * i.e. the "*" and ONLY elements.
+	 */

Hm. I think we probably need that?

+	 * XXX what if there's another event trigger running concurrently that
+	 * renames the schema or moves the object to another schema?  Seems
+	 * pretty far-fetched, but possible nonetheless.
+	 */

We should probably prohibit DDL from within event triggers?

+	switch (node->renameType)
+	{
+		case OBJECT_COLLATION:
+		case OBJECT_CONVERSION:
+		case OBJECT_DOMAIN:
+		case OBJECT_TSDICTIONARY:
+		case OBJECT_TSPARSER:
+		case OBJECT_TSTEMPLATE:
+		case OBJECT_TSCONFIGURATION:
+		case OBJECT_TYPE:
+			{
+				char	   *identity;
+				HeapTuple	objTup;
+				Relation	catalog;
+				Datum		objnsp;
+				bool		isnull;
+				Oid			classId = get_objtype_catalog_oid(node->renameType);
+				AttrNumber	Anum_namespace = get_object_attnum_namespace(classId);
+
+				catalog = relation_open(classId, AccessShareLock);
+				objTup = get_catalog_object_by_oid(catalog, objectId);
+				objnsp = heap_getattr(objTup, Anum_namespace,
+									  RelationGetDescr(catalog), &isnull);
+				if (isnull)
+					elog(ERROR, "invalid NULL namespace");
+
+				identity = psprintf("%s.%s", get_namespace_name(DatumGetObjectId(objnsp)),
+									strVal(llast(node->object)));
+				fmtstr = psprintf("ALTER %s %%{identity}s RENAME TO %%{newname}I",
+								  stringify_objtype(node->renameType));

Is that correct? I.e. will it work for a namespace that needs to be
quoted? There's a couple of these. Afaics they all should use
new_objtree_for_qualname and %{}D? I guess in some cases, like the type,
that could be slightly more complicated, but I guess it has to be done
nonetheless?

+		case OBJECT_AGGREGATE:
+		case OBJECT_FUNCTION:
+			{
+				char	   *newident;
+				ObjectAddress objaddr;
+				const char	   *quoted_newname;
+				StringInfoData old_ident;
+				char	   *start;
+
+				/*
+				 * Generating a function/aggregate identity is altogether too
+				 * messy, so instead of doing it ourselves, we generate one for
+				 * the renamed object, then strip out the name and replace it
+				 * with the original name from the parse node.  This is so ugly
+				 * that we don't dare do it for any other object kind.
+				 */
+
+				objaddr.classId = get_objtype_catalog_oid(node->renameType);
+				objaddr.objectId = objectId;
+				objaddr.objectSubId = 0;
+				newident = getObjectIdentity(&objaddr);
+
+				quoted_newname = quote_identifier(node->newname);
+				start = strstr(newident, quoted_newname);
+				if (!start)
+					elog(ERROR, "could not find %s in %s", start, newident);
+				initStringInfo(&old_ident);
+				appendBinaryStringInfo(&old_ident, newident, start - newident);
+				appendStringInfoString(&old_ident,
+									   quote_identifier(strVal(llast(node->object))));
+				appendStringInfoString(&old_ident, start + strlen(quoted_newname));
+
+				fmtstr = psprintf("ALTER %s %%{identity}s RENAME TO %%{newname}I",
+								  stringify_objtype(node->renameType));
+				renameStmt = new_objtree_VA(fmtstr, 1,
+											"identity", ObjTypeString, old_ident.data);

Yuck.

I think it'd be just as easy to split the argument output from the
function name output in format_procedure_internal, and make that
separately callable.

Subject: [PATCH 18/42] deparse: Support CREATE FUNCTION

/*
+ * deparse_CreateFunctionStmt
+ *		Deparse a CreateFunctionStmt (CREATE FUNCTION)
+ *
+ * Given a function OID and the parsetree that created it, return the JSON
+ * blob representing the creation command.
+ *
+ * XXX this is missing the per-function custom-GUC thing.
+ */

Support attached as 0003.

Subject: [PATCH 19/42] deparse: Support ALTER TABLE

+/*
+ * EventTriggerStartRecordingSubcmds
+ *		Prepare to receive data on a complex DDL command about to be executed
+ *
+ * Note we don't actually stash the object we create here into the "stashed"
+ * list; instead we keep it in curcmd, and only when we're done processing the
+ * subcommands we will add it to the actual stash.
+ *
+ * FIXME -- this API isn't considering the possibility of an ALTER TABLE command
+ * being called reentrantly by an event trigger function.  Do we need stackable
+ * commands at this level?
+ */
+void
+EventTriggerComplexCmdStart(Node *parsetree, ObjectType objtype)
+{
+	MemoryContext	oldcxt;
+	StashedCommand *stashed;
+
+	oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
+
+	stashed = palloc(sizeof(StashedCommand));
+
+	stashed->type = SCT_AlterTable;
+	stashed->in_extension = creating_extension;
+
+	stashed->d.alterTable.objectId = InvalidOid;
+	stashed->d.alterTable.objtype = objtype;
+	stashed->d.alterTable.subcmds = NIL;
+	stashed->parsetree = copyObject(parsetree);
+
+	currentEventTriggerState->curcmd = stashed;
+
+	MemoryContextSwitchTo(oldcxt);
+}

Hm. So +EventTriggerComplexCmdStart is hardcoded to use SCT_AlterTable? That's a bit odd.

+/*
+ * EventTriggerEndRecordingSubcmds
+ * 		Finish up saving a complex DDL command
+ *
+ * FIXME this API isn't considering the possibility that a xact/subxact is
+ * aborted partway through.  Probably it's best to add an
+ * AtEOSubXact_EventTriggers() to fix this.
+ */

I think that'd generally be better than requiring PG_CATCH.

+static ObjTree *
+deparse_AlterTableStmt(StashedCommand *cmd)
+{

+	foreach(cell, cmd->d.alterTable.subcmds)
+	{
+		StashedATSubcmd	*substashed = (StashedATSubcmd *) lfirst(cell);
+		AlterTableCmd	*subcmd = (AlterTableCmd *) substashed->parsetree;
+		ObjTree	   *tree;
+
+		Assert(IsA(subcmd, AlterTableCmd));
+
+		switch (subcmd->subtype)
+		{
+			case AT_AddColumn:
+			case AT_AddColumnRecurse:
+				/* XXX need to set the "recurse" bit somewhere? */
+				Assert(IsA(subcmd->def, ColumnDef));
+				tree = deparse_ColumnDef(rel, dpcontext, false,
+										 (ColumnDef *) subcmd->def);
+				tmp = new_objtree_VA("ADD COLUMN %{definition}s",
+									 2, "type", ObjTypeString, "add column",
+									 "definition", ObjTypeObject, tree);
+				subcmds = lappend(subcmds, new_object_object(tmp));
+				break;

Misses ONLY handling?

+			case AT_AddIndex:
+				{
+					Oid			idxOid = substashed->oid;
+					IndexStmt  *istmt;
+					Relation	idx;
+					const char *idxname;
+					Oid			constrOid;
+
+					Assert(IsA(subcmd->def, IndexStmt));
+					istmt = (IndexStmt *) subcmd->def;
+
+					if (!istmt->isconstraint)
+						break;
+
+					idx = relation_open(idxOid, AccessShareLock);
+					idxname = RelationGetRelationName(idx);
+
+					constrOid = get_relation_constraint_oid(
+						cmd->d.alterTable.objectId, idxname, false);
+
+					tmp = new_objtree_VA("ADD CONSTRAINT %{name}I %{definition}s",
+										 3, "type", ObjTypeString, "add constraint",
+										 "name", ObjTypeString, idxname,
+										 "definition", ObjTypeString,
+										 pg_get_constraintdef_string(constrOid, false));
+					subcmds = lappend(subcmds, new_object_object(tmp));
+
+					relation_close(idx, AccessShareLock);
+				}
+				break;

Hm, didn't you have a out of line version of this somewhere?

+ case AT_AlterColumnType:
+ {

+					/*
+					 * Error out if the USING clause was used.  We cannot use
+					 * it directly here, because it needs to run through
+					 * transformExpr() before being usable for ruleutils.c, and
+					 * we're not in a position to transform it ourselves.  To
+					 * fix this problem, tablecmds.c needs to be modified to store
+					 * the transformed expression somewhere in the StashedATSubcmd.
+					 */
+					tmp2 = new_objtree_VA("USING %{expression}s", 0);
+					if (def->raw_default)
+						elog(ERROR, "unimplemented deparse of ALTER TABLE TYPE USING");
+					else
+						append_bool_object(tmp2, "present", false);
+					append_object_object(tmp, "using", tmp2);

Hm. This probably needs to be fixed :(.

+				}
+				break;
+
+			case AT_AlterColumnGenericOptions:
+				elog(ERROR, "unimplemented deparse of ALTER TABLE ALTER COLUMN OPTIONS");
+				break;

+			case AT_SetRelOptions:
+				elog(ERROR, "unimplemented deparse of ALTER TABLE SET");
+				break;
+
+			case AT_ResetRelOptions:
+				elog(ERROR, "unimplemented deparse of ALTER TABLE RESET");
+				break;

+			case AT_GenericOptions:
+				elog(ERROR, "unimplemented deparse of ALTER TABLE OPTIONS (...)");
+				break;
+

Shouldn't be too hard...

Could you perhaps attach some string that can be searched for to all
commands that you think need to be implemented before being committable?

else
{
-								/* Recurse for anything else */
+								/*
+								 * Recurse for anything else.  If we need to do
+								 * so, "close" the current complex-command set,
+								 * and start a new one at the bottom; this is
+								 * needed to ensure the ordering of queued
+								 * commands is consistent with the way they are
+								 * executed here.
+								 */
+								EventTriggerComplexCmdEnd();
ProcessUtility(stmt,
queryString,
PROCESS_UTILITY_SUBCOMMAND,
params,
None_Receiver,
NULL);
+								EventTriggerComplexCmdStart(parsetree, atstmt->relkind);
+								EventTriggerComplexCmdSetOid(relid);
}

Hm. Commands are always ordered in a way this is ok?

Subject: [PATCH 21/42] deparse: Support CREATE OPERATOR FAMILY

static ObjTree *
+deparse_CreateOpFamily(Oid objectId, Node *parsetree)
+{
+	HeapTuple   opfTup;
+	HeapTuple   amTup;
+	Form_pg_opfamily opfForm;
+	Form_pg_am  amForm;
+	ObjTree	   *copfStmt;
+	ObjTree	   *tmp;
+
+	opfTup = SearchSysCache1(OPFAMILYOID, ObjectIdGetDatum(objectId));
+	if (!HeapTupleIsValid(opfTup))
+		elog(ERROR, "cache lookup failed for operator family with OID %u", objectId);
+	opfForm = (Form_pg_opfamily) GETSTRUCT(opfTup);
+
+	amTup = SearchSysCache1(AMOID, ObjectIdGetDatum(opfForm->opfmethod));
+	if (!HeapTupleIsValid(amTup))
+		elog(ERROR, "cache lookup failed for access method %u",
+			 opfForm->opfmethod);
+	amForm = (Form_pg_am) GETSTRUCT(amTup);
+
+	copfStmt = new_objtree_VA("CREATE OPERATOR FAMILY %{identity}D USING %{amname}s",
+							  0);
+
+	tmp = new_objtree_for_qualname(opfForm->opfnamespace,
+								   NameStr(opfForm->opfname));
+	append_object_object(copfStmt, "identity", tmp);
+	append_string_object(copfStmt, "amname", NameStr(amForm->amname));

Hm. Amname is unquoted? I can't believe this will ever really be a problem, but still.

Subject: [PATCH 24/42] deparse: support ALTER THING OWNER TO
static ObjTree *
+deparse_AlterOwnerStmt(Oid objectId, Node *parsetree)
+{
+	AlterOwnerStmt *node = (AlterOwnerStmt *) parsetree;
+	ObjTree	   *ownerStmt;
+	ObjectAddress addr;
+	char	   *fmt;
+
+	fmt = psprintf("ALTER %s %%{identity}s OWNER TO %%{newname}I",
+				   stringify_objtype(node->objectType));

newname sounds like copied from rename.

Subject: [PATCH 25/42] deparse: Support ALTER EXTENSION / UPDATE TO

Subject: [PATCH 26/42] deparse: Support GRANT/REVOKE

+			else
+			{
+				Assert(cmd->type == SCT_Grant);
+
+				/* classid */
+				nulls[i++] = true;
+				/* objid */
+				nulls[i++] = true;
+				/* objsubid */
+				nulls[i++] = true;

Might actually be nice to fill those out to the object that's being
changed. But it might end up being more work than justified, and it can
be easily added later. I guess it'd also mean we would have to generate
multiple GRANT/REVOKE commands.

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c09915d..92bccf5 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -781,6 +781,7 @@ standard_ProcessUtility(Node *parsetree,
else
ExecuteGrantStmt((GrantStmt *) parsetree);
}
+			break;

Uh. Was that missing from an earlier commit?

case T_DropStmt:
{
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index cdce9e1..60b590b 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -17,6 +17,7 @@
#include "catalog/objectaddress.h"
#include "catalog/pg_event_trigger.h"
#include "nodes/parsenodes.h"
+#include "utils/aclchk.h"

typedef struct EventTriggerData
{
@@ -62,6 +63,7 @@ extern void EventTriggerSQLDropAddObject(const ObjectAddress *object,

extern void EventTriggerStashCommand(Oid objectId, uint32 objectSubId,
ObjectType objtype, Oid secondaryOid, Node *parsetree);
+extern void EventTriggerStashGrant(InternalGrant *istmt);
extern void EventTriggerComplexCmdStart(Node *parsetree, ObjectType objtype);
extern void EventTriggerComplexCmdSetOid(Oid objectId);
extern void EventTriggerRecordSubcmd(Node *subcmd, Oid relid,
diff --git a/src/include/tcop/deparse_utility.h b/src/include/tcop/deparse_utility.h
index 8ebbf34..c364603 100644
--- a/src/include/tcop/deparse_utility.h
+++ b/src/include/tcop/deparse_utility.h
@@ -14,6 +14,8 @@

#include "access/attnum.h"
#include "nodes/nodes.h"
+#include "utils/aclchk.h"
+

/*
* Support for keeping track of a command to deparse.
@@ -26,7 +28,8 @@
typedef enum StashedCommandType
{
SCT_Simple,
- SCT_AlterTable
+ SCT_AlterTable,
+ SCT_Grant
} StashedCommandType;

/*
@@ -61,6 +64,12 @@ typedef struct StashedCommand
ObjectType objtype;
List   *subcmds;
} alterTable;
+
+		struct GrantCommand
+		{
+			InternalGrant *istmt;
+			const char *type;
+		} grant;
} d;
} StashedCommand;

diff --git a/src/include/utils/aclchk.h b/src/include/utils/aclchk.h
new file mode 100644
index 0000000..1ca7095
--- /dev/null
+++ b/src/include/utils/aclchk.h
@@ -0,0 +1,45 @@
+/*-------------------------------------------------------------------------
+ *
+ * aclchk.h
+ *
+ * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/aclchk.h

Maybe we should name it aclchk_internal.h?

Subject: [PATCH 27/42] deparse: Support ALTER FUNCTION

+ * deparse_AlterFunctionStmt
+ *		Deparse a AlterFunctionStmt (ALTER FUNCTION)
+ *
+ * Given a function OID and the parsetree that created it, return the JSON
+ * blob representing the alter command.
+ *
+ * XXX this is missing the per-function custom-GUC thing.
+ */

Hm, so my earlier ptatch needs to partially be copied here. I'm running out of time now though...

+static ObjTree *
+deparse_AlterFunction(Oid objectId, Node *parsetree)
+{

+	foreach(cell, node->actions)
+	{
+		DefElem	*defel = (DefElem *) lfirst(cell);
+		ObjTree	   *tmp = NULL;
+
+		if (strcmp(defel->defname, "volatility") == 0)
+		{
+			tmp = new_objtree_VA(strVal(defel->arg), 0);
+		}
+		else if (strcmp(defel->defname, "strict") == 0)
+		{
+			tmp = new_objtree_VA(intVal(defel->arg) ?
+								 "RETURNS NULL ON NULL INPUT" :
+								 "CALLED ON NULL INPUT", 0);
+		}

Ick. I wish there was NOT STRICT.

Subject: [PATCH 28/42] deparse: support COMMENT ON

+static ObjTree *
+deparse_CommentOnConstraintSmt(Oid objectId, Node *parsetree)
+{
+	CommentStmt *node = (CommentStmt *) parsetree;
+	ObjTree	   *comment;
+	HeapTuple	constrTup;
+	Form_pg_constraint constrForm;
+	char	   *fmt;
+	ObjectAddress addr;
+
+	Assert(node->objtype == OBJECT_TABCONSTRAINT || node->objtype == OBJECT_DOMCONSTRAINT);
+
+	if (node->comment)
+		fmt = psprintf("COMMENT ON CONSTRAINT %%{identity}s ON %s%%{parentobj}s IS %%{comment}L",
+					   node->objtype == OBJECT_TABCONSTRAINT ? "" : "DOMAIN ");
+	else
+		fmt = psprintf("COMMENT ON CONSTRAINT %%{identity}s ON %s%%{parentobj}s IS NULL",
+					   node->objtype == OBJECT_TABCONSTRAINT ? "" : "DOMAIN ");

psprintf(...' IS %s', .... node->comment ? "%{comment}L" : "NULL") maybe?

+
+static ObjTree *
+deparse_CommentStmt(Oid objectId, Oid objectSubId, Node *parsetree)
+{

+	if (node->comment)
+		fmt = psprintf("COMMENT ON %s %%{identity}s IS %%{comment}L",
+					   stringify_objtype(node->objtype));
+	else
+		fmt = psprintf("COMMENT ON %s %%{identity}s IS NULL",
+					   stringify_objtype(node->objtype));

similarly here?

+	comment = new_objtree_VA(fmt, 0);
+	if (node->comment)
+		append_string_object(comment, "comment", node->comment);

Or at leas tthis should be moved into the above if.

Subject: [PATCH 29/42] deparse: support SECURITY LABEL

static ObjTree *
+deparse_SecLabelStmt(Oid objectId, Oid objectSubId, Node *parsetree)
+{
+	SecLabelStmt *node = (SecLabelStmt *) parsetree;
+	ObjTree	   *label;
+	ObjectAddress addr;
+	char	   *fmt;
+
+	if (node->label)
+	{
+		fmt = psprintf("SECURITY LABEL FOR %%{provider}s ON %s %%{identity}s IS %%{label}L",
+				   stringify_objtype(node->objtype));
+		label = new_objtree_VA(fmt, 0);
+
+		append_string_object(label, "label", node->label);
+	}
+	else
+	{
+		fmt = psprintf("SECURITY LABEL FOR %%{provider}s ON %s %%{identity}s IS NULL",
+				   stringify_objtype(node->objtype));
+		label = new_objtree_VA(fmt, 0);
+	}

Should probably merged similarly.

"deparse: Handle default security provider." should be squashed into this.

Subject: [PATCH 34/42] deparse: support CREATE TABLE AS

+static ObjTree *
+deparse_CreateTableAsStmt(Oid objectId, Node *parsetree)
+{

+	/*
+	 * Note that INSERT INTO is deparsed as CREATE TABLE AS.  They are
+	 * functionally equivalent synonyms so there is no harm from this.
+	 */
+	if (node->relkind == OBJECT_MATVIEW)
+		fmt = "CREATE %{persistence}s MATERIALIZED VIEW %{if_not_exists}s "
+			"%{identity}D %{columns}s %{with}s %{tablespace}s "
+			"AS %{query}s %{with_no_data}s";
+	else
+		fmt = "CREATE %{persistence}s TABLE %{if_not_exists}s "
+			"%{identity}D %{columns}s %{with}s %{on_commit}s %{tablespace}s "
+			"AS %{query}s %{with_no_data}s";

With my replication hat on, which I admit isn't very general in this
case, this isn't actually what I want... What I really would rather have
there is a normal CREATE TABLE statement that I can replay on the other
side. But I guess that has to be done somehow in a utility hook :(

+	/* Add an ON COMMIT clause.  CREATE MATERIALIZED VIEW doesn't have one */
+	if (node->relkind == OBJECT_TABLE)
+	{
+		tmp = new_objtree_VA("ON COMMIT %{on_commit_value}s", 0);
+		switch (node->into->onCommit)
+		{
+			case ONCOMMIT_DROP:
+				append_string_object(tmp, "on_commit_value", "DROP");
+				break;
+
+			case ONCOMMIT_DELETE_ROWS:
+				append_string_object(tmp, "on_commit_value", "DELETE ROWS");
+				break;
+
+			case ONCOMMIT_PRESERVE_ROWS:
+				append_string_object(tmp, "on_commit_value", "PRESERVE ROWS");
+				break;
+
+			case ONCOMMIT_NOOP:
+				append_null_object(tmp, "on_commit_value");
+				append_bool_object(tmp, "present", false);
+				break;
+		}
+		append_object_object(createStmt, "on_commit", tmp);
+	}

That switch already exists somewhere else? Maybe add a function that
converts ONCOMMIT_* into the relevant string?

Ran out of energy here...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro, all,

* Alvaro Herrera (alvherre@2ndquadrant.com) 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.)

Alright, I spent a solid few hours yesterday going back through the past
year+ discussion around this. I'm planning to spend more time on
review, though I see Andres has done a good review and I'm generally
on-board with the comments he made. There's a few things which aren't
really "review" but more commentary on the approach that I wanted to
bring up independently.

First off, I took a look at what ends up being required for CREATE
POLICY, as it's code I would have been writing if the timing had ended
up a bit differently. Below is the CREATE POLICY support and it looks
pretty darn reasonable to me and no worse than dealing with pg_dump, or
psql, or the other areas which we have to update whenever we're adding
new commands. To be clear, I'm no more interested in adding more work
to be done whenever we add a new command than the next committer, but
this isn't bad and gives us a new capability (well, almost, more below)
which I've wished for since I started hacking on PG some 13+ years ago.

It'd be *really* nice to be able to pass an object identifier to some
function and get back the CREATE (in particular, though perhaps DROP, or
whatever) command for it. This gets us *awful* close to that without
actually giving it to us and that's bugging me. The issue is the
parsetree, which I understand may be required in some cases (ALTER,
primairly, it seems?), but isn't always.

Looking at the CREATE POLICY patch, here's what I see.

diff --git a/src/backend/tcop/deparse_utility.c b/src/backend/tcop/deparse_utility.c
index 2a0a2b9..cd600ff 100644
--- a/src/backend/tcop/deparse_utility.c
+++ b/src/backend/tcop/deparse_utility.c
@@ -4402,6 +4402,91 @@ deparse_CommentStmt(Oid objectId, Oid objectSubId, Node *parsetree)
}

static ObjTree *
+deparse_CreatePolicyStmt(Oid objectId, Node *parsetree)
+{
+	CreatePolicyStmt *node = (CreatePolicyStmt *) parsetree;
+	ObjTree	   *policy;
+	ObjTree	   *tmp;
+	Relation	polRel = heap_open(PolicyRelationId, AccessShareLock);
+	HeapTuple	polTup = get_catalog_object_by_oid(polRel, objectId);
+	Form_pg_policy polForm;
+	ListCell   *cell;
+	List	   *list;
+
+	if (!HeapTupleIsValid(polTup))
+		elog(ERROR, "cache lookup failed for policy %u", objectId);
+	polForm = (Form_pg_policy) GETSTRUCT(polTup);

Alright, we get the parsetree and build a CreatePolicyStmt with it, but
what do we use it for? We also get the polForm from pg_policy.

+	policy = new_objtree_VA("CREATE POLICY %{identity}I ON %{table}D %{for_command}s "
+							"%{to_role}s %{using}s %{with_check}s", 1,
+							"identity", ObjTypeString, node->policy_name);

The policy_name is certainly available from pg_policy instead.

+	/* add the "ON table" clause */
+	append_object_object(policy, "table",
+						 new_objtree_for_qualname_id(RelationRelationId,
+													 polForm->polrelid));

Here we're getting the polrelid from the polForm.

+	/* add "FOR command" clause */
+	tmp = new_objtree_VA("FOR %{command}s", 1,
+						 "command", ObjTypeString, node->cmd);
+	append_object_object(policy, "for_command", tmp);

The cmd is also available from the polForm.

+	/* add the "TO role" clause. Always contains at least PUBLIC */
+	tmp = new_objtree_VA("TO %{role:, }I", 0);
+	list = NIL;
+	foreach (cell, node->roles)
+	{
+		list = lappend(list,
+					   new_string_object(strVal(lfirst(cell))));
+	}
+	append_array_object(tmp, "role", list);
+	append_object_object(policy, "to_role", tmp);
+
+	/* add the USING clause, if any */
+	tmp = new_objtree_VA("USING (%{expression}s)", 0);
+	if (node->qual)
+	{
+		Datum	deparsed;
+		Datum	storedexpr;
+		bool	isnull;
+
+		storedexpr = heap_getattr(polTup, Anum_pg_policy_polqual,
+								  RelationGetDescr(polRel), &isnull);
+		if (isnull)
+			elog(ERROR, "invalid NULL polqual expression in policy %u", objectId);
+		deparsed = DirectFunctionCall2(pg_get_expr, storedexpr, polForm->polrelid);
+		append_string_object(tmp, "expression",
+							 TextDatumGetCString(deparsed));
+	}
+	else
+		append_bool_object(tmp, "present", false);
+	append_object_object(policy, "using", tmp);
+
+	/* add the WITH CHECK clause, if any */
+	tmp = new_objtree_VA("WITH CHECK (%{expression}s)", 0);
+	if (node->with_check)
+	{
+		Datum	deparsed;
+		Datum	storedexpr;
+		bool	isnull;
+
+		storedexpr = heap_getattr(polTup, Anum_pg_policy_polwithcheck,
+								  RelationGetDescr(polRel), &isnull);
+		if (isnull)
+			elog(ERROR, "invalid NULL polwithcheck expression in policy %u", objectId);
+		deparsed = DirectFunctionCall2(pg_get_expr, storedexpr, polForm->polrelid);
+		append_string_object(tmp, "expression",
+							 TextDatumGetCString(deparsed));
+	}
+	else
+		append_bool_object(tmp, "present", false);
+	append_object_object(policy, "with_check", tmp);
+
+	heap_close(polRel, AccessShareLock);

The USING and WITH CHECK quals can be extracted from the polForm also,
of course, it's what psql and pg_dump are doing, after all.

So, why depend on the parsetree? What about have another argument which
a user could use without the parsetree, for things that it's absolutely
required for today (eg: ALTER sub-command differentiation)? Maybe
that's possible and maybe it isn't, but at least for the CREATE cases we
should be able to avoid forcing a user to provide a built parsetree, and
that'd be *really* nice and open up this feature to quite a few other
use-cases that I can think of. I'd further suggest that we provide
these command to the SQL level, and then have a wrapper which will take
the name of an object, resolve it to Oid, and then pass back the CREATE
command for it.

For as much commentary as there has been about event triggers and
replication and BDR, and about how this is going to end up being a
little-used side-feature, I'm amazed that there has been very little
discussion about how this would finally put into the backend the ability
to build up CREATE commands for objects and remove the need to use
pg_dump for that (something which isn't exactly fun for GUI applications
and other use-cases). Further, this would increase the number of people
using the actually complicated part of this, which is building up of the
command from the catalog, and that helps address both the concerns about
bad lurking bugs that go unnoticed and about how this wouldn't be used
very much and therefore might bitrot.

I don't know about the rest of you, but I'd love to see a psql backslash
command to compliment this capability, to which you can specify a table
and get back a CREATE TABLE statement.

And, yes, I'm fine with the JSON aspect of this (even though I realize
my above commentary glosses over it) and think that's likely to be
picked up by other clients also- so we should certainly provide a way to
expose that structure to clients too (imagine pgAdmin4 built on top of
this!).

Thanks!

Stephen

On 2015-02-21 14:51:32 -0500, Stephen Frost wrote:

It'd be *really* nice to be able to pass an object identifier to some
function and get back the CREATE (in particular, though perhaps DROP, or
whatever) command for it. This gets us *awful* close to that without
actually giving it to us and that's bugging me. The issue is the
parsetree, which I understand may be required in some cases (ALTER,
primairly, it seems?), but isn't always.

The USING and WITH CHECK quals can be extracted from the polForm also,
of course, it's what psql and pg_dump are doing, after all.

So, why depend on the parsetree? What about have another argument which
a user could use without the parsetree, for things that it's absolutely
required for today (eg: ALTER sub-command differentiation)?

I'm really not wild about pretty massively expanding the scope at the
eleventh hour. There's a fair number of commands where this the
deparsing command will give you a bunch of commands - look at CREATE
TABLE and CREATE SCHEMA ... for the most extreme examples. For those
there's no chance to do that without the parse tree available.

Yes, it might be possible to use the same code for a bunch of minor
commands, but not for the interesting/complex stuff.

Maybe that's possible and maybe it isn't, but at least for the CREATE
cases we should be able to avoid forcing a user to provide a built
parsetree, and that'd be *really* nice and open up this feature to
quite a few other use-cases that I can think of. I'd further suggest
that we provide these command to the SQL level, and then have a
wrapper which will take the name of an object, resolve it to Oid, and
then pass back the CREATE command for it.

I think this is a different feature that might end up sharing some of
the infrastructure, but not more.

For as much commentary as there has been about event triggers and
replication and BDR, and about how this is going to end up being a
little-used side-feature, I'm amazed that there has been very little
discussion about how this would finally put into the backend the ability
to build up CREATE commands for objects and remove the need to use
pg_dump for that (something which isn't exactly fun for GUI applications
and other use-cases).

I don't think it's all that related, so I'm not surprised. If you
execute a CREATE TABLE(id serial primary key); you'll get a bunch of
commands with this facility - it'd be much less clear what exactly shall
be dumped in the case of some hypothetical GUI when you want to dump the
table.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2015-02-21 17:30:24 +0100, Andres Freund wrote:

/*
+ * deparse_CreateFunctionStmt
+ *		Deparse a CreateFunctionStmt (CREATE FUNCTION)
+ *
+ * Given a function OID and the parsetree that created it, return the JSON
+ * blob representing the creation command.
+ *
+ * XXX this is missing the per-function custom-GUC thing.
+ */

Subject: [PATCH 27/42] deparse: Support ALTER FUNCTION

+ * deparse_AlterFunctionStmt
+ *		Deparse a AlterFunctionStmt (ALTER FUNCTION)
+ *
+ * Given a function OID and the parsetree that created it, return the JSON
+ * blob representing the alter command.
+ *
+ * XXX this is missing the per-function custom-GUC thing.
+ */

Hm, so my earlier ptatch needs to partially be copied here. I'm running out of time now though...

Updated 0003 attached that supports both SET for both CREATE and
ALTER. In my previous patch I'd looked at proconfig for the values. A
bit further thought revealed that that's not such a great idea: It works
well enough for CREATE FUNCTION, but already breaks down at CREATE OR
REPLACE FUNCTION unless we want to emit RESET ALL (SET ...)+ which seems
mighty ugly.

Also, the code is actually a good bit easier to understand this
way. I. hate. arrays. ;)

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres,

* Andres Freund (andres@2ndquadrant.com) wrote:

On 2015-02-21 14:51:32 -0500, Stephen Frost wrote:

It'd be *really* nice to be able to pass an object identifier to some
function and get back the CREATE (in particular, though perhaps DROP, or
whatever) command for it. This gets us *awful* close to that without
actually giving it to us and that's bugging me. The issue is the
parsetree, which I understand may be required in some cases (ALTER,
primairly, it seems?), but isn't always.

The USING and WITH CHECK quals can be extracted from the polForm also,
of course, it's what psql and pg_dump are doing, after all.

So, why depend on the parsetree? What about have another argument which
a user could use without the parsetree, for things that it's absolutely
required for today (eg: ALTER sub-command differentiation)?

I'm really not wild about pretty massively expanding the scope at the
eleventh hour. There's a fair number of commands where this the
deparsing command will give you a bunch of commands - look at CREATE
TABLE and CREATE SCHEMA ... for the most extreme examples. For those
there's no chance to do that without the parse tree available.

For my 2c, I don't agree with either of your assumptions above. I don't
see this as a massive expansion of the scope, nor that we're in the 11th
hour at this point. Agreed, it's the last commitfest, but we're near
the beginning of it and Alvaro has already made modifications to the
patch set, as is generally expected to happen for any patch in a
commitfest, without much trouble. The changes which I'm suggesting are
nearly trivial for the larger body of work and only slightly more than
trivial for the more complicated pieces.

Yes, it might be possible to use the same code for a bunch of minor
commands, but not for the interesting/complex stuff.

We can clearly rebuild at least CREATE commands for all objects without
access to the parse tree, obviously pg_dump manages somehow. I didn't
specify that a single command had to be used. Further, the actual
deparse_CreateStmt doesn't look like it'd have a terribly hard time
producing something without access to the parsetree.

The whole premise of this approach is that we're using the results of
the catalog changes as the basis for what's needed to recreate the
command. The places where we're referring to the parsetree look more
like a crutch of convenience than for any particular good reason to.
Consider this part of 0011 which includes deparse_CreateStmt:

/*
* Process table elements: column definitions and constraints. Only
* the column definitions are obtained from the parse node itself. To
* get constraints we rely on pg_constraint, because the parse node
* might be missing some things such as the name of the constraints.
*/

and then looking down through to deparse_ColumnDef, we see that the
ColumnDef passed in is used almost exclusivly for the *column name*
(which is used to look up the entry in pg_attribute..). Looping over
the pg_attribute entries for the table in the attnum order would
certainly return the same result, or something has gone wrong.

Beyond the inheiritance consideration, the only other reference actually
leads to what you argued (correctly, in my view) was a mistake in the
code- where a NOT NULL is omitted for the primary key case. Weren't you
also complaining about the 'OF type' form being a potential issue?
That'd also go away with the approach I'm advocating. In short, I
suspect that if this approach had been taken originally, at least some
of the concerns and issued levied against the current implementation
wouldn't exist. Thankfully, the way the code has been developed, the
majority of the code is general infrastructure and the changes I'm
suggesting are all in code which is simpler, thanks to that
infrastructure already being there.

All I'm suggesting is that we focus on collecting the information from
the catalog and avoid using the parsetree. For at least the CREATE and
DROP cases, that should be entirely possible. This does end up being a
bit different from the original goal, which was closer to "reproduce
exactly the command that was specified," but as shown above, that
probably wasn't what we ever really wanted. The original command is
ambiguous on a number of levels and even where it isn't we can get the
canonical information we need straight from the catalog.

Maybe that's possible and maybe it isn't, but at least for the CREATE
cases we should be able to avoid forcing a user to provide a built
parsetree, and that'd be *really* nice and open up this feature to
quite a few other use-cases that I can think of. I'd further suggest
that we provide these command to the SQL level, and then have a
wrapper which will take the name of an object, resolve it to Oid, and
then pass back the CREATE command for it.

I think this is a different feature that might end up sharing some of
the infrastructure, but not more.

I know you've looked through this code also and I really don't get the
comment that only "some" of this infrastructure would be shared. As I
tried to point out, for the 'CREATE POLICY' case, a few minor
modifications would have it provide exactly what I'm suggesting and I'm
sure that most of the cases would be similar. Simply looking through
the code with an eye towards avoiding the parsetree in favor of pulling
information from the catalog would illustrate the point I'm making, I
believe.

For as much commentary as there has been about event triggers and
replication and BDR, and about how this is going to end up being a
little-used side-feature, I'm amazed that there has been very little
discussion about how this would finally put into the backend the ability
to build up CREATE commands for objects and remove the need to use
pg_dump for that (something which isn't exactly fun for GUI applications
and other use-cases).

I don't think it's all that related, so I'm not surprised. If you
execute a CREATE TABLE(id serial primary key); you'll get a bunch of
commands with this facility - it'd be much less clear what exactly shall
be dumped in the case of some hypothetical GUI when you want to dump the
table.

I really don't think it's all that strange or complicated to consider
and we've got a rather nice example of what a good approach would be.

This may only apply to the CREATE and DROP cases, but that's no small
thing.

Apologies for taking so long to reply, it wasn't intentional.
Unfortunately, I'm not going to have a lot of time tomorrow either but I
hope to find time later in the week to resume review and perhaps to
provide code to back the approach I'm advocating.

Thanks!

Stephen