Re: [HACKERS] deparsing utility commands

2015-08-20 Thread Alvaro Herrera
Shulgin, Oleksandr wrote: Another quirk of ALTER TABLE is that due to multi-pass processing in ATRewriteCatalogs, the same command might be collected a number of times. For example, in src/test/regress/sql/inherit.sql: alter table a alter column aa type integer using bit_length(aa); the

Re: [HACKERS] deparsing utility commands

2015-08-20 Thread Alvaro Herrera
Shulgin, Oleksandr wrote: A particularly nasty one is: ERROR: index cwi_replaced_pkey does not exist The test statement that's causing it is this one: ALTER TABLE cwi_test DROP CONSTRAINT cwi_uniq_idx, ADD CONSTRAINT cwi_replaced_pkey PRIMARY KEY USING INDEX cwi_uniq2_idx; Which

Re: [HACKERS] deparsing utility commands

2015-08-20 Thread Shulgin, Oleksandr
On Thu, Aug 20, 2015 at 4:28 PM, Shulgin, Oleksandr oleksandr.shul...@zalando.de wrote: Which gets deparsed as: ALTER TABLE cwi_test DROP CONSTRAINT cwi_uniq_idx, ADD CONSTRAINT cwi_replaced_pkey PRIMARY KEY USING INDEX cwi_replaced_pkey; The problem is that during constraint

Re: [HACKERS] deparsing utility commands

2015-08-06 Thread Alvaro Herrera
Jim Nasby wrote: FWIW, my interest in this is largely due to the problems I've had with this in the variant type. In particular, using the same resolution rules for functions and operators. So I'm wondering if there's a bigger issue here. I'd be glad to review your variant stuff, but I doubt

Re: [HACKERS] deparsing utility commands

2015-08-06 Thread Jim Nasby
On 8/5/15 9:55 PM, Alvaro Herrera wrote: Jim Nasby wrote: On 7/31/15 8:45 AM, Shulgin, Oleksandr wrote: STATEMENT: create view v1 as select * from t1 ; ERROR: operator does not exist: pg_catalog.oid = pg_catalog.oid at character 52 HINT: No operator matches the given name and argument

Re: [HACKERS] deparsing utility commands

2015-08-05 Thread Jim Nasby
On 7/31/15 8:45 AM, Shulgin, Oleksandr wrote: While running deparsecheck suite I'm getting a number of oddly looking errors: WARNING: state: 42883 errm: operator does not exist: pg_catalog.oid = pg_catalog.oid This is caused by deparsing create view, e.g.: STATEMENT: create view v1 as

Re: [HACKERS] deparsing utility commands

2015-08-05 Thread Alvaro Herrera
Jim Nasby wrote: On 7/31/15 8:45 AM, Shulgin, Oleksandr wrote: STATEMENT: create view v1 as select * from t1 ; ERROR: operator does not exist: pg_catalog.oid = pg_catalog.oid at character 52 HINT: No operator matches the given name and argument type(s). You might need to add explicit

Re: [HACKERS] deparsing utility commands

2015-07-31 Thread Shulgin, Oleksandr
On Wed, Jul 29, 2015 at 12:44 PM, Shulgin, Oleksandr oleksandr.shul...@zalando.de wrote: On Tue, May 12, 2015 at 12:25 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: David Steele wrote: I have reviewed and tested this patch and everything looks good to me. It also looks like you

Re: [HACKERS] deparsing utility commands

2015-07-29 Thread Shulgin, Oleksandr
On Tue, May 12, 2015 at 12:25 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: David Steele wrote: I have reviewed and tested this patch and everything looks good to me. It also looks like you added better coverage for schema DDL, which is a welcome addition. Thanks -- I have pushed

Re: [HACKERS] deparsing utility commands

2015-05-11 Thread David Steele
On 5/8/15 3:29 PM, Alvaro Herrera wrote: Here is a complete version. Barring serious problems, I intend to commit this on Monday. I have reviewed and tested this patch and everything looks good to me. It also looks like you added better coverage for schema DDL, which is a welcome addition. --

Re: [HACKERS] deparsing utility commands

2015-05-11 Thread Alvaro Herrera
David Steele wrote: I have reviewed and tested this patch and everything looks good to me. It also looks like you added better coverage for schema DDL, which is a welcome addition. Thanks -- I have pushed this now. My hope is to get this test module extended quite a bit, not only to cover

Re: [HACKERS] deparsing utility commands

2015-05-08 Thread Alvaro Herrera
Alvaro Herrera wrote: Here's a cleaned up version of this patch; I threw together a very quick test module, and updated a conflicting OID. As far as I can tell, I'm only missing the documentation updates before this is push-able. Here is a complete version. Barring serious problems, I intend

Re: [HACKERS] deparsing utility commands

2015-05-08 Thread Fabrízio de Royes Mello
On Fri, May 8, 2015 at 3:14 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Alvaro Herrera wrote: Here's a cleaned up version of this patch; I threw together a very quick test module, and updated a conflicting OID. As far as I can tell, I'm only missing the documentation updates before

Re: [HACKERS] deparsing utility commands

2015-05-08 Thread Alvaro Herrera
Alvaro Herrera wrote: Alvaro Herrera wrote: Here's a cleaned up version of this patch; I threw together a very quick test module, and updated a conflicting OID. As far as I can tell, I'm only missing the documentation updates before this is push-able. Here is a complete version.

Re: [HACKERS] deparsing utility commands

2015-05-04 Thread Alvaro Herrera
Here's a cleaned up version of this patch; I threw together a very quick test module, and updated a conflicting OID. As far as I can tell, I'm only missing the documentation updates before this is push-able. One change to note is that the AlterTable support used to ignore commands that didn't

Re: [HACKERS] deparsing utility commands

2015-04-28 Thread Dimitri Fontaine
Hi, As a comment to the whole email below, I think the following approach is the best compromise we will find, allowing everybody to come up with their own format much as in the Logical Decoding plugins world. Of course, as in the Logical Decoding area, BDR and similar projects will implement

Re: [HACKERS] deparsing utility commands

2015-04-14 Thread David Steele
On 4/9/15 12:14 PM, Alvaro Herrera wrote: Alvaro Herrera wrote: Executive summary: There is now a CommandDeparse_hook; deparse_utility_command is provided as an extension, intended for 9.6; rest of patch would be pushed to 9.5. Actually here's another approach I like better: use a new

Re: [HACKERS] deparsing utility commands

2015-04-08 Thread David Steele
On 4/7/15 4:32 PM, Alvaro Herrera wrote: Executive summary: There is now a CommandDeparse_hook; deparse_utility_command is provided as an extension, intended for 9.6; rest of patch would be pushed to 9.5. Long version: I've made command deparsing hookable. Attached there are three

Re: [HACKERS] deparsing utility commands

2015-04-07 Thread Alvaro Herrera
Executive summary: There is now a CommandDeparse_hook; deparse_utility_command is provided as an extension, intended for 9.6; rest of patch would be pushed to 9.5. Long version: I've made command deparsing hookable. Attached there are three patches: the first patch contains changes to core

Re: [HACKERS] deparsing utility commands

2015-04-02 Thread Robert Haas
On Wed, Mar 25, 2015 at 3:21 PM, Ryan Pedela rped...@datalanche.com wrote: On Wed, Mar 25, 2015 at 11:59 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: * Should we prohibit DDL from within event triggers? Please don't prohibit DDL unless there is a really, really good reason to do so. I

Re: [HACKERS] deparsing utility commands

2015-04-02 Thread Andres Freund
On 2015-04-02 12:05:13 -0400, Robert Haas wrote: On Wed, Mar 25, 2015 at 3:21 PM, Ryan Pedela rped...@datalanche.com wrote: On Wed, Mar 25, 2015 at 11:59 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: * Should we prohibit DDL from within event triggers? Please don't prohibit DDL

Re: [HACKERS] deparsing utility commands

2015-03-25 Thread Ryan Pedela
On Wed, Mar 25, 2015 at 11:59 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: * Should we prohibit DDL from within event triggers? Please don't prohibit DDL unless there is a really, really good reason to do so. I have several use cases in mind for event triggers, but they are only useful

Re: [HACKERS] deparsing utility commands

2015-03-25 Thread Alvaro Herrera
Alvaro Herrera wrote: Here's an updated version of this series. I just pushed patches 0001 and 0002, with very small tweaks; those had already been reviewed and it didn't seem like there was much controversy. To test the posted series it's probably easiest to git checkout

Re: [HACKERS] deparsing utility commands

2015-03-18 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: One thing that Stephen commented on was the ALTER TABLE preparatory patch. He said why not return ObjectAddress from the subcommand routines instead of just Oid/attnum? The reason is that to interpret the address correctly you will still

Re: [HACKERS] deparsing utility commands

2015-03-05 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Thanks for the review. I have pushed these patches, squashed in one commit, with the exception of the one that changed ALTER TABLE. You had enough comments about that one that I decided to put it aside for now, and get the other

Re: [HACKERS] deparsing utility commands

2015-03-03 Thread Alvaro Herrera
Stephen, Thanks for the review. I have pushed these patches, squashed in one commit, with the exception of the one that changed ALTER TABLE. You had enough comments about that one that I decided to put it aside for now, and get the other ones done. I will post a new series shortly. I fixed

Re: [HACKERS] deparsing utility commands

2015-03-02 Thread Andres Freund
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

Re: [HACKERS] deparsing utility commands

2015-02-27 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Thanks. I have adjusted the code to use ObjectAddress in all functions called by ProcessUtilitySlow; the patch is a bit tedious but seems pretty reasonable to me. As I said, there is quite a lot of churn. Thanks for doing this!

Re: [HACKERS] deparsing utility commands

2015-02-24 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: Regardless, as I've tried to point out above, the changes which I'm actually suggesting for this initial body of work are just to avoid the parsetree and go based off of what the catalog has. I'm hopeful that's a

Re: [HACKERS] deparsing utility commands

2015-02-24 Thread Stephen Frost
* Andres Freund (and...@2ndquadrant.com) wrote: Hi, On 2015-02-23 19:48:43 -0500, Stephen Frost wrote: 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

Re: [HACKERS] deparsing utility commands

2015-02-24 Thread Ryan Pedela
On Tue, Feb 24, 2015 at 6:48 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Stephen Frost wrote: I'm thinking something like SELECT * FROM pg_creation_commands({'pg_class'::regclass, 'sometable'::pg_class}); would return a set of commands in the JSON-blob format that creates the

Re: [HACKERS] deparsing utility commands

2015-02-24 Thread Alvaro Herrera
Stephen Frost wrote: Regardless, as I've tried to point out above, the changes which I'm actually suggesting for this initial body of work are just to avoid the parsetree and go based off of what the catalog has. I'm hopeful that's a small enough and reasonable enough change to happen during

Re: [HACKERS] deparsing utility commands

2015-02-24 Thread Andres Freund
On 2015-02-24 10:48:38 -0300, Alvaro Herrera wrote: Stephen Frost wrote: Regardless, as I've tried to point out above, the changes which I'm actually suggesting for this initial body of work are just to avoid the parsetree and go based off of what the catalog has. I'm hopeful that's a

Re: [HACKERS] deparsing utility commands

2015-02-23 Thread Stephen Frost
Andres, * Andres Freund (and...@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

Re: [HACKERS] deparsing utility commands

2015-02-23 Thread Andres Freund
Hi, On 2015-02-23 19:48:43 -0500, Stephen Frost wrote: 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

Re: [HACKERS] deparsing utility commands

2015-02-22 Thread Andres Freund
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

Re: [HACKERS] deparsing utility commands

2015-02-21 Thread Andres Freund
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

Re: [HACKERS] deparsing utility commands

2015-02-21 Thread Andres Freund
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[] =

Re: [HACKERS] deparsing utility commands

2015-02-21 Thread Andres Freund
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

Re: [HACKERS] deparsing utility commands

2015-02-21 Thread Andres Freund
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

Re: [HACKERS] deparsing utility commands

2015-02-21 Thread Stephen Frost
Alvaro, all, * Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-19 Thread Alvaro Herrera
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),

Re: [HACKERS] deparsing utility commands

2015-02-19 Thread Alvaro Herrera
Stephen Frost wrote: * Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Alvaro Herrera
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.

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Alvaro Herrera
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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Alvaro Herrera
Stephen, Stephen Frost wrote: * Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: * Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Stephen Frost
* Alvaro Herrera (alvhe...@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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Alvaro Herrera
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

Re: [HACKERS] deparsing utility commands

2015-02-18 Thread Andres Freund
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