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 alter column type then appears 4 times in the deparsed output as
 identical subcommands of a single ALTER TABLE command.

Yeah, I had a hack somewhere in the collection code that if the relation
ID was different from what was specified, then the command was ignored.
I removed that before commit because it seemed possible that for some
cases you actually want the command reported separately for each child.

I think our best option in this case is to ignore commands that are
reported for different relations during JSON deparsing.  Not sure how
easy that is.

-- 
Álvaro Herrerahttp://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


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 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 transformation, the index is being
 renamed to match the constraint name and at the deparse stage the original
 index name appears to be lost completely...  I haven't figure out if
 there's a way around unless we introduce a new field in IndexStmt
 (originalName?) to cover exactly this corner case.

Oh :-(  Hmm, modifying parse nodes should normally be considered last
resort, and at the same time identifying objects based on names rather
than OIDs is frowned upon.  But I don't see any way to handle this
otherwise.  I'm not sure what's the best solution for this one.

 The updated versions of the core-support patch and the contrib modules are
 attached.

Please try to split things up, or at least don't mix more than it
already is.  For instance, the tsconfig mapping stuff should be its own
patch; we can probably make a case for pushing that on its own.

Also I see you added one caller of EventTriggerInhibitCommandCollection.
I don't like that crap at all and I think we would be better off if we
were able to remove it completely.  Please see whether it's possible to
handle this case in some other way.

You seem to have squashed the patches?  Please keep the split out.

-- 
Álvaro Herrerahttp://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


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 transformation, the index is being
 renamed to match the constraint name and at the deparse stage the original
 index name appears to be lost completely...  I haven't figure out if
 there's a way around unless we introduce a new field in IndexStmt
 (originalName?) to cover exactly this corner case.

 The updated versions of the core-support patch and the contrib modules are
 attached.


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 alter column type then appears 4 times in the deparsed output as
identical subcommands of a single ALTER TABLE command.


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 it's the same
thing at play.  Please don't hijack this thread.  Deparse is doing some
pretty nasty games with the backend innards to make the deparsed
representation usable generally.  We were just missing a simple trick.

-- 
Álvaro Herrerahttp://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


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 type(s). You
might need to add explicit type casts.
QUERY:  SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND
rulename = $2
CONTEXT:  PL/pgSQL function test_ddl_deparse() line 1 at FOR over SELECT
rows



I'm not sure what test_ddl_deparse is doing, is that where the oid = oid is
coming from?

It might be enlightening to replace = with OPERATOR(pg_catalog.=) and see if
that works.


That worked, thanks!


Good, but why weren't operator resolution rules working correctly here 
to begin with?


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.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com


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


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 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 type casts.
QUERY:  SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND
rulename = $2
CONTEXT:  PL/pgSQL function test_ddl_deparse() line 1 at FOR over SELECT
rows

The pg_rewrite query comes from ruleutils.c, while ddl_deparse.c calls
it through pg_get_viewdef_internal() but don't understand how is it
different from e.g., select pg_get_viewdef(...), and that last one is
not affected.


I'm not sure what test_ddl_deparse is doing, is that where the oid = oid 
is coming from?


It might be enlightening to replace = with OPERATOR(pg_catalog.=) and 
see if that works.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com


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


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 type casts.
 QUERY:  SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND
 rulename = $2
 CONTEXT:  PL/pgSQL function test_ddl_deparse() line 1 at FOR over SELECT
 rows

 I'm not sure what test_ddl_deparse is doing, is that where the oid = oid is
 coming from?
 
 It might be enlightening to replace = with OPERATOR(pg_catalog.=) and see if
 that works.

That worked, thanks!

-- 
Álvaro Herrerahttp://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


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 added better coverage for schema DDL, which is a
  welcome addition.

 Thanks -- I have pushed this now.


 Hi,

 I've tried compiling the 0003-ddl_deparse-extension part from
 http://www.postgresql.org/message-id/20150409161419.gc4...@alvh.no-ip.org
 on current master and that has failed because the 0002 part hasn't been
 actually pushed (I've asked Alvaro off the list about this, that's how I
 know the reason ;-).

 I was able to update the 0002 part so it applies cleanly (updated version
 attached), and then the contrib module compiles after one minor change and
 seems to work.

 I've started to look into what it would take to move 0002's code to the
 extension itself, and I've got a question about use of printTypmod() in
 format_type_detailed():

 if (typemod = 0)
 *typemodstr = printTypmod(NULL, typemod, typeform-typmodout);
 else
 *typemodstr = pstrdup();

 Given that printTypmod() does psprintf(%s%s) one way or the other,
 shouldn't we pass an empty string here instead of NULL as typname argument?


Testing shows this is a bug indeed, easily triggered by deparsing any type
with typmod.

My hope is to get this test module extended quite a bit, not only to
 cover existing commands, but also so that it causes future changes to
 cause failure unless command collection is considered.  (In a previous
 version of this patch, there was a test mode that ran everything in the
 serial_schedule of regular regression tests.  That was IMV a good way to
 ensure that new commands were also tested to run under command
 collection.  That hasn't been enabled in the new test module, and I
 think it's necessary.)



 If anyone wants to contribute to the test module so that more is
 covered, that would be much appreciated.


 I'm planning to have a look at this part also.


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 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 type casts.
QUERY:  SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND
rulename = $2
CONTEXT:  PL/pgSQL function test_ddl_deparse() line 1 at FOR over SELECT
rows

The pg_rewrite query comes from ruleutils.c, while ddl_deparse.c calls it
through pg_get_viewdef_internal() but don't understand how is it different
from e.g., select pg_get_viewdef(...), and that last one is not affected.

Thoughts?

--
Alex


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


Hi,

I've tried compiling the 0003-ddl_deparse-extension part from
http://www.postgresql.org/message-id/20150409161419.gc4...@alvh.no-ip.org
on current master and that has failed because the 0002 part hasn't been
actually pushed (I've asked Alvaro off the list about this, that's how I
know the reason ;-).

I was able to update the 0002 part so it applies cleanly (updated version
attached), and then the contrib module compiles after one minor change and
seems to work.

I've started to look into what it would take to move 0002's code to the
extension itself, and I've got a question about use of printTypmod() in
format_type_detailed():

if (typemod = 0)
*typemodstr = printTypmod(NULL, typemod, typeform-typmodout);
else
*typemodstr = pstrdup();

Given that printTypmod() does psprintf(%s%s) one way or the other,
shouldn't we pass an empty string here instead of NULL as typname argument?

My hope is to get this test module extended quite a bit, not only to
 cover existing commands, but also so that it causes future changes to
 cause failure unless command collection is considered.  (In a previous
 version of this patch, there was a test mode that ran everything in the
 serial_schedule of regular regression tests.  That was IMV a good way to
 ensure that new commands were also tested to run under command
 collection.  That hasn't been enabled in the new test module, and I
 think it's necessary.)



 If anyone wants to contribute to the test module so that more is
 covered, that would be much appreciated.


I'm planning to have a look at this part also.

--
Alex
From 5381f5efafd1c2fd29b7c842e5ef1edd552d545e Mon Sep 17 00:00:00 2001
From: Oleksandr Shulgin oleksandr.shul...@zalando.de
Date: Wed, 29 Jul 2015 11:09:56 +0200
Subject: [PATCH] ddl_deparse core support

---
 src/backend/commands/seclabel.c |   5 +
 src/backend/commands/sequence.c |  34 +++
 src/backend/commands/tablecmds.c|   3 +-
 src/backend/utils/adt/format_type.c | 127 +
 src/backend/utils/adt/regproc.c | 101 +--
 src/backend/utils/adt/ruleutils.c   | 516 
 src/include/commands/sequence.h |   1 +
 src/include/utils/builtins.h|   4 +
 src/include/utils/ruleutils.h   |  21 +-
 9 files changed, 730 insertions(+), 82 deletions(-)

diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 1ef98ce..aa4de97 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -63,6 +63,11 @@ ExecSecLabelStmt(SecLabelStmt *stmt)
 	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 	 errmsg(must specify provider when multiple security label providers have been loaded)));
 		provider = (LabelProvider *) linitial(label_provider_list);
+		/*
+		 * Set the provider in the statement so that DDL deparse can use
+		 * provider explicitly in generated statement.
+		 */
+		stmt-provider = (char *) provider-provider_name;
 	}
 	else
 	{
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 9c1037f..b0f8003 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1517,6 +1517,40 @@ process_owned_by(Relation seqrel, List *owned_by)
 		relation_close(tablerel, NoLock);
 }
 
+/*
+ * Return sequence parameters, detailed
+ */
+Form_pg_sequence
+get_sequence_values(Oid sequenceId)
+{
+	Buffer		buf;
+	SeqTable	elm;
+	Relation	seqrel;
+	HeapTupleData seqtuple;
+	Form_pg_sequence seq;
+	Form_pg_sequence retSeq;
+
+	retSeq = palloc(sizeof(FormData_pg_sequence));
+
+	/* open and AccessShareLock sequence */
+	init_sequence(sequenceId, elm, seqrel);
+
+	if (pg_class_aclcheck(sequenceId, GetUserId(),
+		  ACL_SELECT | ACL_UPDATE | ACL_USAGE) != ACLCHECK_OK)
+		ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg(permission denied for sequence %s,
+		RelationGetRelationName(seqrel;
+
+	seq = read_seq_tuple(elm, seqrel, buf, seqtuple);
+
+	memcpy(retSeq, seq, sizeof(FormData_pg_sequence));
+
+	UnlockReleaseBuffer(buf);
+	relation_close(seqrel, NoLock);
+
+	return retSeq;
+}
 
 /*
  * Return sequence parameters, for use by information schema
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d394713..5058f9f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8169,7 +8169,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 if (!list_member_oid(tab-changedConstraintOids,
 	 foundObject.objectId))
 {
-	char	   *defstring = pg_get_constraintdef_string(foundObject.objectId);
+	char	   *defstring = pg_get_constraintdef_string(foundObject.objectId,
+	

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.

-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature


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 existing commands, but also so that it causes future changes to
cause failure unless command collection is considered.  (In a previous
version of this patch, there was a test mode that ran everything in the
serial_schedule of regular regression tests.  That was IMV a good way to
ensure that new commands were also tested to run under command
collection.  That hasn't been enabled in the new test module, and I
think it's necessary.)

If anyone wants to contribute to the test module so that more is
covered, that would be much appreciated.

-- 
Álvaro Herrerahttp://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


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 to
commit this on Monday.

-- 
Álvaro Herrerahttp://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


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 this is push-able.

 Here is a complete version.  Barring serious problems, I intend to
 commit this on Monday.


You forgot to attach the patches!

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog: http://fabriziomello.github.io
 Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
 Github: http://github.com/fabriziomello


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.  Barring serious problems, I intend to
 commit this on Monday.


-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index fb39731..04762e8 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18270,6 +18270,97 @@ CREATE EVENT TRIGGER test_table_rewrite_oid
 /programlisting
 /para
   /sect2
+
+  sect2 id=pg-event-trigger-ddl-command-end-functions
+   titleCapturing Changes at Command End/title
+
+   indexterm
+primarypg_event_trigger_ddl_commands/primary
+   /indexterm
+
+   para
+functionpg_event_trigger_ddl_commands/ returns a list of
+acronymDDL/acronym (data definition language) commands executed by
+each user action, when invoked in a function attached to a
+literalddl_command_end/ event trigger.  If called in any other
+context, an error is raised.
+functionpg_event_trigger_ddl_commands/ returns one row for each
+base command executed; some commands that are a single SQL sentence
+may return more than one row.  This function returns the following
+columns:
+
+informaltable
+ tgroup cols=3
+  thead
+   row
+entryName/entry
+entryType/entry
+entryDescription/entry
+   /row
+  /thead
+
+  tbody
+   row
+entryliteralclassid/literal/entry
+entrytypeOid/type/entry
+entryOID of catalog the object belongs in/entry
+   /row
+   row
+entryliteralobjid/literal/entry
+entrytypeOid/type/entry
+entryOID of the object in the catalog/entry
+   /row
+   row
+entryliteralobjsubid/literal/entry
+entrytypeinteger/type/entry
+entryObject sub-id (e.g. attribute number for columns)/entry
+   /row
+   row
+entryliteralcommand_tag/literal/entry
+entrytypetext/type/entry
+entrycommand tag/entry
+   /row
+   row
+entryliteralobject_type/literal/entry
+entrytypetext/type/entry
+entryType of the object/entry
+   /row
+   row
+entryliteralschema_name/literal/entry
+entrytypetext/type/entry
+entry
+ Name of the schema the object belongs in, if any; otherwise literalNULL/.
+ No quoting is applied.
+/entry
+   /row
+   row
+entryliteralobject_identity/literal/entry
+entrytypetext/type/entry
+entry
+ Text rendering of the object identity, schema-qualified. Each and every
+ identifier present in the identity is quoted if necessary.
+/entry
+   /row
+   row
+entryliteralin_extension/literal/entry
+entrytypebool/type/entry
+entrywhether the command is part of an extension script/entry
+   /row
+   row
+entryliteralcommand/literal/entry
+entrytypepg_ddl_command/type/entry
+entry
+ A complete representation of the command, in internal format.
+ This cannot be output directly, but it can be passed to other
+ functions to obtain different pieces of information about the
+ command.
+/entry
+   /row
+  /tbody
+ /tgroup
+/informaltable
+   /para
+  /sect2
   /sect1
 
 /chapter
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 8e75c27..943909c 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -48,6 +48,7 @@
 #include catalog/pg_ts_config.h
 #include catalog/pg_ts_dict.h
 #include commands/dbcommands.h
+#include commands/event_trigger.h
 #include commands/proclang.h
 #include commands/tablespace.h
 #include foreign/foreign.h
@@ -56,6 +57,7 @@
 #include parser/parse_func.h
 #include parser/parse_type.h
 #include utils/acl.h
+#include utils/aclchk_internal.h
 #include utils/builtins.h
 #include utils/fmgroids.h
 #include utils/lsyscache.h
@@ -65,32 +67,6 @@
 
 
 /*
- * The information about one Grant/Revoke statement, in internal format: object
- * and grantees names have been turned into Oids, the privilege list is an
- * AclMode bitmask.  If 'privileges' is ACL_NO_RIGHTS (the 0 value) and
- * all_privs is true, 'privileges' will be internally set to the right kind of
- * ACL_ALL_RIGHTS_*, depending on the object type (NB - this will modify the
- * InternalGrant struct!)
- *
- * Note: 'all_privs' and 'privileges' represent object-level privileges only.
- * There might also be column-level privilege specifications, which are
- * represented in col_privs (this is a list of untransformed AccessPriv nodes).
- * Column privileges are only valid for objtype ACL_OBJECT_RELATION.
- 

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 match the OID as set by
EventTriggerAlterTableRelid(); the comment there said that the point was
to avoid collecting the same commands to child tables as recursion
occured in execution.  I think that would be imposing such a decision;
perhaps some users of this infrastructure want to know about the
operations as they happen on child tables too.  With this definition it
is up to the user module to ignore the duplicates.

Thanks for your review.  In reply to your comments:

Amit Kapila wrote:

 Few Comments/Questions regrading first 2 patches:
 
 Regarding Patch 0001-deparse-infrastructure-needed-for-command-deparsing
 
 1.
 + * Currently, sql_drop, table_rewrite, ddL_command_end events are the only
 
 /ddL_command_end/ddl_command_end
 
 'L' should be in lower case.

True. Fixed.

 2.
 + * 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.
 + */
 +void
 +EventTriggerAlterTableEnd(void)
 {
 ..
 }
 
 Wouldn't the same fix be required for RollbackToSavePoint case
 as well?  Do you intend to fix this in separate patch?

Acrually, I figured that this is not at issue.  When a subxact is rolled
back, the whole currentEventTriggerState thing is reverted to a previous
item in the stack; if an event trigger is fired by ALTER TABLE, and the
resulting function invokes ALTER TABLE again, they collect their
commands in separate state elements, so there is never a conflict.
I can't think of any situation in which one event trigger function adds
elements to the list of the calling command.


 3.
 + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
 + * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
 
 Copyright notice years should be same.

Yeah, fixed.

 4.
 + /*
 + * copying the node is moderately challenging ... should we consider
 + * changing InternalGrant into a full-fledged node instead?
 + */
 + icopy = palloc(sizeof(InternalGrant));
 + memcpy(icopy, istmt, sizeof(InternalGrant));
 + icopy-objects = list_copy(istmt-objects);
 
 Don't we need to copy (AclMode privileges;)?

AFAICT that's copied by memcpy.

 5.
 -static void AlterOpFamilyAdd(List *opfamilyname, Oid amoid, Oid
 opfamilyoid,
 +static void AlterOpFamilyAdd(AlterOpFamilyStmt *stmt,
 + List *opfamilyname, Oid amoid, Oid opfamilyoid,
   int maxOpNumber, int maxProcNumber,
   List *items);
 -static void AlterOpFamilyDrop(List *opfamilyname, Oid amoid, Oid
 opfamilyoid,
 +static void AlterOpFamilyDrop(AlterOpFamilyStmt *stmt,
 +  List *opfamilyname, Oid amoid, Oid opfamilyoid,
int maxOpNumber, int maxProcNumber,
List *items);
 
 Now that both the above functions have parameter AlterOpFamilyStmt *stmt,
 so can't we get rid of second parameter List *opfamilyname as that
 is part of structure AlterOpFamilyStmt?

Yeah, I considered that as I wrote the code but dropped it out of
laziness.  I have done so now.

 6.
 @@ -1175,204 +1229,258 @@ ProcessUtilitySlow(Node *parsetree,
 ..
 + EventTriggerAlterTableStart(parsetree);
 + address =
 + DefineIndex(relid, /* OID of heap relation */
 + stmt,
 + InvalidOid, /* no predefined OID */
 + false, /* is_alter_table */
 + true, /* check_rights */
 + false, /* skip_build */
 + false); /* quiet */
 + /*
 + * Add the CREATE INDEX node itself to stash right away; if
 + * there were any commands stashed in the ALTER TABLE code,
 + * we need them to appear after this one.
 + */
 + EventTriggerCollectSimpleCommand(address, secondaryObject,
 + parsetree);
 + commandCollected = true;
 + EventTriggerAlterTableEnd();
 
 Is there a reason why EventTriggerAlterTableRelid() is not called
 after EventTriggerAlterTableStart() in this flow?

All paths that go through AlterTableInternal() have the Oid set by that
function.

 7.
 +void
 +EventTriggerInhibitCommandCollection(void)
 
 +void
 +EventTriggerUndoInhibitCommandCollection(void)
 
 These function names are understandable, some alternative names could be
 InhibitEventTriggerCommandCollection(),
 PreventEventTriggerCommandCollection(),
 or ProhibitEventTriggerCommandCollection() if you prefer anyone of these
 over others.

Hmm, most of the reason I picked these names is the EventTrigger prefix.

 8.
   case T_CreateOpClassStmt:
 - DefineOpClass((CreateOpClassStmt *) parsetree);
 + address = DefineOpClass((CreateOpClassStmt *) parsetree);
 + /* command is stashed in DefineOpClass */
 + commandCollected = true;
 
 Is there a need to remember address if command is already
 collected?

None.  Removed that.

 Regarding Patch 0002-changes-to-core-to-support-the-deparse-extension:

I decided against committing 0002 patch for 

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 their own representation, in BDR IIUC the DDL will get
translated into a JSON based AST thing.

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Actually here's another approach I like better: use a new pseudotype,
 pg_ddl_command, which internally is just a pointer to a CollectedCommand
 struct.  We cannot give access to the pointer directly in SQL, so much
 like type internal or pg_node_tree the in/out functions should just
 error out.  But the type can be returned as a column in
 pg_event_trigger_ddl_command.  An extension can create a function that
 receives that type as argument, and return a different representation of
 the command; the third patch creates a function ddl_deparse_to_json()
 which does that.

 You can have as many extensions as you want, and your event triggers can
 use the column as many times as necessary.  This removes the limitation
 of the previous approach that you could only have a single extension
 providing a CommandDeparse_hook function.

 This patch applies cleanly on top of current master.  You need to
 install the extension with CREATE EXTENSION ddl_deparse after building
 it in contrib.

 Note: the extension is NOT to be committed to core, only the first two
 patches; that will give us more room to revisit the JSON representation
 more promptly.  My intention is that the extension will be published
 elsewhere.

+1 from me,

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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


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 pseudotype,
 pg_ddl_command, which internally is just a pointer to a CollectedCommand
 struct.  We cannot give access to the pointer directly in SQL, so much
 like type internal or pg_node_tree the in/out functions should just
 error out.  But the type can be returned as a column in
 pg_event_trigger_ddl_command.  An extension can create a function that
 receives that type as argument, and return a different representation of
 the command; the third patch creates a function ddl_deparse_to_json()
 which does that.

I've tested this approach and it returns the same results as the
previous approach for all calls to pg_event_trigger_ddl_command() made
by the pg_audit regression tests.  For my testing I was generally only
applying patch 0001 and 0002 (although 0001 on it's own also worked for
my case).  I applied patch 0003 only for the purpose of being sure it
did not break anything, and did not specifically test the functionality
of 0002 or 0003.

However, I've tested 0001 over a very wide range of commands and have
not found any anomalous behaviors.

I've reviewed the 0001 and 0002 patches and don't have anything to add
beyond what has been mentioned in previous reviews.  You've applied your
pattern consistently across a wide variety of utility commands which is
something of a feat.

Since 0003 will not be committed to core, I was more interested in the
mechanism used to connect 0003 to 0001 and 0002.  I like the new
approach better than the old one in that there is no connecting hook
now.  As you say, it provides more flexibility in terms of using the
command data in as many ways as you like.  Even better, the user has
access to three different representations and can choose which one is
best for them.  And, of course, they can write code for their own
representation using pg_ddl_command, JSON, or SQL.

I also like the way the patch has been broken up.  The core
functionality will finally make event triggers truly useful in a
high-level language like pl/pgsql.  I can see many uses for them now
that pg_event_trigger_ddl_command() is present.

From my review and testing, I see no barriers to committing patches 0001
and 0002.  I'd love to see this functionality in 9.5.

Regards,
-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature


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 patches:
 the first patch contains changes to core that just add the command
 list stuff, so that on ddl_command_end there is access to what has been
 executed.  This includes the OID of the object just created, the command
 tag, and assorted other details; the deparsed command in JSON format is
 not immediately part of the result.

I'm looking forward to testing this with pg_audit.  I think the first
patch alone will give us the bulk of the desired functionality.

 The third patch contains all the deparse code, packaged as a contrib
 module and extension named ddl_deparse.  Essentially, it's everything
 that was previously in tcop/deparse_utility.c and utils/adt/ddl_json.c:
 the stuff that takes the parsenode and OID of a command execution and
 turns it into a JSON blob, and also the support function that takes the
 JSON blob and converts back into the plain text rendering of the
 command.
 
 The second patch contains some changes to core code that support the
 ddl_deparse extension; mainly some ruleutils.c changes.

 What links patches 0001 and 0003 is a hook, CommandDeparse_hook.  If
 unset, the pg_event_trigger_ddl_commands function returns some
 boilerplate text like no deparse function installed; if the extension
 is installed, the JSON rendering is returned instead and can be used
 with the ddl_deparse_expand_command() function.

I would prefer for the column to be NULL or to have a boolean column
that indicates if JSON output is available (or both).  I'd rather not do
string matching if I can help it (or test for invalid JSON).

 The rationale for doing things this way is that it will be useful to
 have 9.5 expose the pg_event_trigger_ddl_commands() function for various
 uses, while we refine the JSON bits some more and get it committed for
 9.6.  In reviews, it's clear that there's some more bits to fiddle so
 that it can be as general as possible.  I think we should label the
 whole DDL command reporting as experimental in 9.5 and subject to
 change, so that we can just remove the hook in 9.6 when the ddl_deparse
 thing becomes part of core.

I'm completely on board with this.  I think deparse is a cool piece of
code and I see a bunch of potential uses for it, but at the same time
I'm not sure it has finished baking yet.  This is a smart and safe choice.

Is there a particular commit you'd recommend for applying the deparse
patches (especially the first set)?

-- 
- David Steele
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature


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 that just add the command
list stuff, so that on ddl_command_end there is access to what has been
executed.  This includes the OID of the object just created, the command
tag, and assorted other details; the deparsed command in JSON format is
not immediately part of the result.

The third patch contains all the deparse code, packaged as a contrib
module and extension named ddl_deparse.  Essentially, it's everything
that was previously in tcop/deparse_utility.c and utils/adt/ddl_json.c:
the stuff that takes the parsenode and OID of a command execution and
turns it into a JSON blob, and also the support function that takes the
JSON blob and converts back into the plain text rendering of the
command.

The second patch contains some changes to core code that support the
ddl_deparse extension; mainly some ruleutils.c changes.

What links patches 0001 and 0003 is a hook, CommandDeparse_hook.  If
unset, the pg_event_trigger_ddl_commands function returns some
boilerplate text like no deparse function installed; if the extension
is installed, the JSON rendering is returned instead and can be used
with the ddl_deparse_expand_command() function.

The rationale for doing things this way is that it will be useful to
have 9.5 expose the pg_event_trigger_ddl_commands() function for various
uses, while we refine the JSON bits some more and get it committed for
9.6.  In reviews, it's clear that there's some more bits to fiddle so
that it can be as general as possible.  I think we should label the
whole DDL command reporting as experimental in 9.5 and subject to
change, so that we can just remove the hook in 9.6 when the ddl_deparse
thing becomes part of core.

Thoughts?

-- 
Álvaro Herrerahttp://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


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 have several use cases in mind for event triggers, but they are only
 useful if I can perform DDL.

 For example, I want to create an Elasticsearch FDW and use that to index and
 search Postgres tables. When a table is created, I am planning to use an
 event trigger to capture the CREATE event and automatically create a foreign
 table via the Elasticsearch FDW. In addition, I would add normal triggers to
 the new table which capture DML and update the foreign table accordingly. In
 other words, I want to use FDWs and event triggers to automatically sync
 table DDL and DML to Elasticsearch.

+1.  I think that if it's unsafe to run DDL from with event triggers,
then that might be a sign that it's not safe to run *any* user-defined
code at that location.  I think it's the job of the event trigger to
make sure that it doesn't assume anything about what may have changed
while the user-defined code was running.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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


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 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 if I can perform DDL.
 
  For example, I want to create an Elasticsearch FDW and use that to index and
  search Postgres tables. When a table is created, I am planning to use an
  event trigger to capture the CREATE event and automatically create a foreign
  table via the Elasticsearch FDW. In addition, I would add normal triggers to
  the new table which capture DML and update the foreign table accordingly. In
  other words, I want to use FDWs and event triggers to automatically sync
  table DDL and DML to Elasticsearch.
 
 +1.  I think that if it's unsafe to run DDL from with event triggers,
 then that might be a sign that it's not safe to run *any* user-defined
 code at that location.  I think it's the job of the event trigger to
 make sure that it doesn't assume anything about what may have changed
 while the user-defined code was running.

First of: I don't see a fundamental reason to forbid it, I think it's
just simpler to analyze that way. But I'm unconvinced by that
reasoning. As you know we prohibit executing DDL on some objects in
*lots* of places c.f. CheckTableNotInUse(), without that imo being a
sign of a more fundamental problem.

Greetings,

Andres Freund


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


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 if I can perform DDL.

For example, I want to create an Elasticsearch FDW and use that to index
and search Postgres tables. When a table is created, I am planning to use
an event trigger to capture the CREATE event and automatically create a
foreign table via the Elasticsearch FDW. In addition, I would add normal
triggers to the new table which capture DML and update the foreign table
accordingly. In other words, I want to use FDWs and event triggers to
automatically sync table DDL and DML to Elasticsearch.


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 b3196e65f5bfc997ec7fa3f91645a09289c10dee
and apply it all on top of that.

-- 
Álvaro Herrerahttp://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


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 need a lot more context; the OID
 and attnum are only part of the truth anyway.  I think there are a small
 number of cases where we could meaningfully return an ObjectAddress and
 have the whole thing be useful, but I'm not sure it's worth the bother.

Alright, fair enough.

 In patch 0004, I removed most of the Stash() calls in ProcessUtility,
 instead adding one at the bottom that takes care of most of the simple
 cases.  That means that a developer adding support for something new in
 ProcessUtilitySlow without realizing that something must be added to the
 command stash will get an error fairly early, which I think is helpful.

Right, I like this.

 Patch 0021 (fixing a bug in SECURITY LABEL support) I'm not really sure
 about.  I don't like modifying a parse node during execution -- seems a
 bad habit.  It seems better to me to return the chosen security label as
 an ObjectAddress in ExecSecLabelStmt, as pass that as secondaryOid to
 the deparse_utility.c routine.

I agree, changing a parse node during execution definitely seems like a
bad idea, particularly if there's a way you could avoid doing so, which
it sounds like there is.  Any reason to not follow up on that?

 In patch 0023 (CREATE/ALTER POLICY support) I ran into trouble.  I
 represent the role in the json like this:
   tmp = new_objtree_VA(TO %{role:, }I, 0);
 which means that role names are quoted as identifiers.  This means it
 doesn't work as soon as we get a command referencing PUBLIC (which many
 in the regression test do, because when the TO clause is omitted, PUBLIC
 is the default).  So this ends up as PUBLIC and everything goes
 downhill.  I'm not sure what to do about this, except to invent a new
 %{} code.

How is this any different from the GRANT/REVOKE cases..?  Surely you
have to deal with 'public' being special there also.

 Subject: [PATCH 01/29] add secondaryOid to DefineTSConfiguration()

This looks fine to me.

 Subject: [PATCH 02/29] deparse/core: have ALTER TABLE return table OID and
  other data
[...]
 diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
 index 5ce4395..9675d21 100644
 @@ -1853,7 +1853,7 @@ heap_drop_with_catalog(Oid relid)
  /*
   * Store a default expression for column attnum of relation rel.
   */
 -void
 +Oid
  StoreAttrDefault(Relation rel, AttrNumber attnum,
Node *expr, bool is_internal)
  {

Missing a comment about the return value?

 @@ -2074,7 +2083,7 @@ StoreConstraints(Relation rel, List 
 *cooked_constraints, bool is_internal)
   int numchecks = 0;
   ListCell   *lc;
  
 - if (!cooked_constraints)
 + if (list_length(cooked_constraints) == 0)
   return; /* nothing to do */
  
   /*

While this is used in a few places, it's by far more common to use:

if (cooked_constraints == NIL)

or to keep it the way it is..


 diff --git a/src/backend/commands/tablecmds.c 
 b/src/backend/commands/tablecmds.c
 index 623e6bf..fc2c8a9 100644
 @@ -3416,78 +3418,95 @@ static void
   case AT_DropColumn: /* DROP COLUMN */
   ATExecDropColumn(wqueue, rel, cmd-name,
 -  cmd-behavior, false, false, 
 cmd-missing_ok, lockmode);
 +  cmd-behavior, false, 
 false,
 +  cmd-missing_ok, 
 lockmode);
   break;
   case AT_DropColumnRecurse:  /* DROP COLUMN with 
 recursion */
   ATExecDropColumn(wqueue, rel, cmd-name,
 -   cmd-behavior, true, false, 
 cmd-missing_ok, lockmode);
 +  cmd-behavior, true, 
 false,
 +  cmd-missing_ok, 
 lockmode);
   break;

[...]

   case AT_ReplicaIdentity:
 - ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) 
 cmd-def, lockmode);
 + ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) 
 cmd-def,
 +   lockmode);
   break;
   case AT_EnableRowSecurity:
   ATExecEnableRowSecurity(rel);

I realize the whole thing is changing anyway, but the random whitespace
changes don't make reviewing any easier.

 -static void
 +static AttrNumber
  ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
   ColumnDef *colDef, bool isOid,
   bool recurse, 

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 ones done.  I will post a new series shortly.

I look forward to seeing the new series posted soon.

 I fixed (most of?) the issues you pointed out; some additional comments:

These all looked good to me.

Thanks!

Stephen


signature.asc
Description: Digital signature


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 (most of?) the issues you pointed out; some additional comments:

Stephen Frost wrote:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:

  @@ -1004,6 +1004,10 @@ AddNewRelationType(const char *typeName,
* use_user_acl: TRUE if should look for user-defined default permissions;
* if FALSE, relacl is always set NULL
* allow_system_table_mods: TRUE to allow creation in system namespaces
  + * is_internal: is this a system-generated catalog?
 
 Shouldn't adding the comment for is_internal be done independently?
 It's clearly missing in master and fixing that hasn't got anything to do
 with the other changes happening here.

That was pushed separately and backpatched to 9.3.  Turns out it was my
own very old mistake.

 This patch did make me think that there's quite a few places which could
 use ObjectAddressSet() that aren't (even after this patch).  I don't
 think we need to stress over that, but it might be nice to mark that
 down as a TODO for someone to go through all the places where we
 construct an ObjectAddress and update them to use the new macro.

Agreed.  I fixed the ones in these patches, but of course there's a lot
of them in other places.


  @@ -772,7 +775,7 @@ DefineOpFamily(CreateOpFamilyStmt *stmt)
* other commands called ALTER OPERATOR FAMILY exist, but go through
* different code paths.
*/
  -Oid
  +void
   AlterOpFamily(AlterOpFamilyStmt *stmt)
   {
  Oid amoid,  /* our AM's oid */
  @@ -826,8 +829,6 @@ AlterOpFamily(AlterOpFamilyStmt *stmt)
  AlterOpFamilyAdd(stmt-opfamilyname, amoid, opfamilyoid,
   maxOpNumber, maxProcNumber,
   stmt-items);
  -
  -   return opfamilyoid;
   }
 
 This feels a bit funny to me..?  Why not construct and return an
 ObjectAddress like the other Alter functions?

The reason for not changing AlterOpFamily is that it needs completely
separate support for event triggers -- a simple ObjectAddress is not
enough.  If you had a look at the support for GrantStmt in the rest of
the series, you have an idea of what supporting this one needs: we need
to store additional information somewhere in the event trigger queue.
Most likely, this function will end up returning an ObjectAddress as
well, but I want to write the deparse code first to make sure the change
will make sense.

In the committed patch, I ended up not changing this from Oid to void;
that was just pointless churn.

  --- a/src/backend/commands/extension.c
  +++ b/src/backend/commands/extension.c
  @@ -2402,7 +2402,7 @@ extension_config_remove(Oid extensionoid, Oid 
  tableoid)
* Execute ALTER EXTENSION SET SCHEMA
*/
   ObjectAddress
  -AlterExtensionNamespace(List *names, const char *newschema)
  +AlterExtensionNamespace(List *names, const char *newschema, Oid *oldschema)
   {
  char   *extensionName;
  Oid extensionOid;
 
 Output parameter is not an ObjectAddress for this one?  The other case
 where a schema was an output parameter, it was..  Feels a little odd.

What's going on here is that this function is not called directly by
ProcessUtilitySlow, but rather through ExecAlterObjectSchemaStmt(); it
seemed simpler to keep the OID return here, and only build the
ObjectAddress when needed.  I did it that way because there's a path
through AlterExtensionNamespace itself that goes back through
AlterObjectNamespace_oid that requires the OID, and the address would
just be clutter most of the time.  It seemed simpler.  Not that much of
an issue anyhow, since this only affects four functions or so; it's
easily changed if necessary.

 Also, the function comment needs updating to reflect the new output
 parameter.

A large number of these functions were not documenting their APIs at
all, and most of them seemed self-explanatory.  I updated the ones for
which it seemed worthwhile.

 Not sure if it's most helpful for me to continue to provide reviews or
 if it'd be useful for me to help with actually making some of these
 changes- please let me know your thoughts and I'll do my best to help.

Given the number of patches in the series, I think it's easiest for both
you and for me to give comments.  I rebase this stuff pretty frequently
for submission.

-- 
Álvaro Herrerahttp://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


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 up in a rather large number of patches, but
 my intention would be to commit separately only the first 6 patches; the
 remaining parts are split up only so that it's easy to see how deparsing
 each patch is implemented, and would be committed as a single changeset
 (but before that I need a bunch of extra fixes and additions to other
 parts.)

I find the split of the patches not really helpful - it makes
fixing/cleaning up things unnecessarily complicated. I'd like most of
the individual command commits. Obviously the preparatory stuff that'll
be independely committed should stay separate. I also like that the
infrastructure patch is split of; same with the more wildly different
patches like support for GRANT.

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


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!

 Also attached are some patches to make some commands return info about
 a secondary object regarding the execution, which can be used to know
 more about what's being executed:
 
 1) ALTER DOMAIN ADD CONSTRAINT returns (in addition to the address of
 the doamin) the address of the new constraint.
 
 2) ALTER OBJECT SET SCHEMA returns (in addition to the address of the
 object) the address of the old schema.
 
 3) ALTER EXTENSION ADD/DROP OBJECT returns (in addition to the address
 of the extension) the address of the added/dropped object.
 
 
 Also attached is the part of these patches that have ALTER TABLE return
 the AttrNumber and OID of the affected object.
 
 I think this is all mostly uninteresting, but thought I'd throw it out
 for public review before pushing, just in case.  I have fixed many
 issues in the rest of the branch meanwhile, and it's looking much better
 IMO, but much work remains there and will leave them for later.

Glad to hear that things are progressing well!

 Subject: [PATCH 1/7] Have RENAME routines return ObjectAddress rather than OID
[...]

  /*
   *   renameatt   - changes the name of a attribute in a 
 relation
 + *
 + * The returned ObjectAddress is that of the pg_class address of the column.
   */
 -Oid
 +ObjectAddress
  renameatt(RenameStmt *stmt)

This comment didn't really come across sensibly to me.  I'd suggest
instead:

The ObjectAddress returned is that of the column which was renamed.

Or something along those lines instead?

The rest of this patch looked fine to me.

 Subject: [PATCH 2/7] deparse/core: have COMMENT return an ObjectAddress, not 
 OID
[...]

Looked fine to me.

 Subject: [PATCH 3/7] deparse/core: have ALTER TABLE return table OID and 
 other data
[...]

 @@ -1844,7 +1844,7 @@ heap_drop_with_catalog(Oid relid)
  /*
   * Store a default expression for column attnum of relation rel.
   */
 -void
 +Oid
  StoreAttrDefault(Relation rel, AttrNumber attnum,
Node *expr, bool is_internal)
  {
 @@ -1949,6 +1949,8 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
*/
   InvokeObjectPostCreateHookArg(AttrDefaultRelationId,
 
 RelationGetRelid(rel), attnum, is_internal);
 +
 + return attrdefOid;
  }

Why isn't this returning an ObjectAddress too?  It even builds one up
for you in defobject.  Also, the comment should be updated to reflect
that it's now returning something.

  /*
 @@ -1956,8 +1958,10 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
   *
   * Caller is responsible for updating the count of constraints
   * in the pg_class entry for the relation.
 + *
 + * The OID of the new constraint is returned.
   */
 -static void
 +static Oid
  StoreRelCheck(Relation rel, char *ccname, Node *expr,
 bool is_validated, bool is_local, int inhcount,
 bool is_no_inherit, bool is_internal)
 @@ -1967,6 +1971,7 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
   List   *varList;
   int keycount;
   int16  *attNos;
 + Oid constrOid;
  
   /*
* Flatten expression to string form for storage.
 @@ -2018,7 +2023,7 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
   /*
* Create the Check Constraint
*/
 - CreateConstraintEntry(ccname,   /* Constraint Name */
 + constrOid = CreateConstraintEntry(ccname,   /* Constraint 
 Name */
 RelationGetNamespace(rel),
 /* namespace */
 CONSTRAINT_CHECK, 
 /* Constraint Type */
 false,/* Is 
 Deferrable */
 @@ -2049,11 +2054,15 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
  
   pfree(ccbin);
   pfree(ccsrc);
 +
 + return constrOid;
  }

Ditto here?  CreateConstraintEntry would have to be updated to return
the ObjectAddress that it builds up, but there aren't all that many
callers of it..

  /*
   * Store defaults and constraints (passed as a list of CookedConstraint).
   *
 + * Each CookedConstraint struct is modified to store the new catalog tuple 
 OID.
 + *
   * NOTE: only pre-cooked expressions will be passed this way, which is to
   * say expressions inherited from an existing relation.  Newly parsed
   * expressions can be added later, by direct calls to StoreAttrDefault
 @@ -2065,7 +2074,7 @@ StoreConstraints(Relation rel, List 
 *cooked_constraints, bool is_internal)
   int numchecks = 0;
   ListCell   *lc;

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 small enough and reasonable enough change to happen
  during this commitfest.  I don't have any issue tabling the rest of the
  discussion and future work based on that to a later time.
 
 At some point I did consider the idea that the CREATE cases of the
 deparse code could be usable without a parse tree.  For the specific
 case of deparsing a DDL command as it's being ran, it's important to
 have the parse tree: options such as IF EXISTS, OR REPLACE and others
 don't live in the catalogs and so they must be obtained from the parse
 tree.  I think it is possible to adjust some of the code so that it can
 run with a NULL parse tree, and set the options to empty in those cases;
 that should return a usable command to re-create the object.

Right, being able to run it with a NULL parse tree would absolutely work
for the use-cases that I'm thinking of.  I suggested somewhere up-thread
that those bits of information (IF EXISTS, OR REPLACE) might be possible
to pass in as independent arguments or through a compound argument (eg:
a List or struct or something), instead of coming from the parse tree,
but that's clearly something which could be done later.

 One point to bear in mind is that there is still some work left; and if
 I need to additionally add extra interfaces so that this code can be
 called from outside event triggers, I will not have any time to review
 other people's patches from the commitfest.  So here's my suggestion:
 let this be pushed with the current interfaces only, with an eye towards
 not adding obstacles to future support for a different interface.
 That's more in the spirit of incremental improvement than trying to cram
 everything in the initial commit.

I'm perfectly fine with having the initial commit of this work using the
existing interfaces and further that we only need one set of interfaces
for 9.5, but, for my part, I'd love to be able to build an extension on
9.5 which calls these functions and passes in a NULL parsetree.  Perhaps
that's wishful thinking at this part and maybe I'll have to wait til
9.6, but that's why I've brought this up.

 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
 table.  The input argument is an array of object addresses, so that you
 can request creation commands for multiple objects.  (It's not my
 intention to provide this functionality right away, but if somebody else
 wants to work on top of the current deparse patch, by my guest; if it
 proves simple enough we can still get it into 9.5 as part of this
 patch.)

I'm all for this, but it seems too late for it to get into 9.5.  I'd be
happy with just the functions existing and available to extensions for
9.5.

Thanks!

Stephen


signature.asc
Description: Digital signature


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 objects without
  access to the parse tree, obviously pg_dump manages somehow.
 
 That's not the same. pg_dump recreates a static state, not running log
 of changes that can be replayed.

There isn't anything particularly special about these function, that I
can see at least, when it comes to a 'running log' vs. 'static state'.

  I didn't specify that a single command had to be used.
 
 Well, if you want to get 'the create table statement' you'll need to
 decide what you want. With ddl event triggers it's clear - something
 that, when replayed, results in the same catalog contents. Such an easy
 definition doesn't, as far as I know, exist for a set of functions you
 seem to imagine.

This is really an orthogonal consideration.  I'd draw a parallel to
CREATE TABLE .. LIKE, where you have to specify what you want.  Perhaps
that's reason enough for this initial version to not be exposed out to
psql, but it doesn't change the point that going based off of the
catalog and not the parsetree would facilitate this capability.
Requiring the parsetree precludes any progress in this direction.

  Further, the actual deparse_CreateStmt doesn't look like it'd have a
  terribly hard time producing something without access to the
  parsetree.
 
 The interesting bit isn't the deparse_CreateStmt itself, but all the
 stuff that's also triggered when you do a CREATE TABLE
 nontrival-stuff;. utility.c will first transform the statement and then
 run run and stash every created subcommand. And the subcommands will
 *also* get deparsed.

Right, all of that is completely independent of deparse_CreateStmt and
there's nothing in your argument for why deparse_CreateStmt should get
access to or need the parsetree.  If I'm following correctly, your
argument is that we shouldn't care that deparse_CreateStmt gets the
parsetree because of how it's getting called and the argument I'm making
is that deparse_CreateStmt really doesn't need the parsetree and if it
wasn't required then deparse_CreateStmt could serve additional
use-cases.

 Just think about what to do about CREATE TABLE foo(id serial primary
 key, data text, bar_id REFERENCES foo.bar); - there's no way you can get
 which other objects to dump from the catalog alone. What for a schema,
 considering CREATE SCHEMA ... (schema_elements)+;?
 
 Sure, individual subcommands could refer to the catalog instead of the
 parse tree. But to get the whole thing you can't easily just refer to
 it.

I'm not entirely following this last sentence, but I'm encouragerd by
the agreement (if I understand correctly) that the subcommands could use
the catalog instead of the parsetree.  The question of if that makes
them useful for other callers is perhaps up for some debate, but they
certainly look valuable from here.  They may need to get extended to
have other options passed in (include defaults, include constraints,
etc) which then cause other sub-commands to be called (or perhaps
there's a larger function overall which is called and handles those
pieces and combines the results and the sub-command remains the same)
but all of that is good and interesting discussion which can happen if
we write these functions without the parsetree.  With the parsetree
requirement, we can't really even have those discussions.

  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.
 
 DROP's already in 9.4, the additions in 9.5 were more or less usability
 things.  The problem generating DROPs is right now more identifying
 which one you want to drop and checking the dependencies - the latter
 I'm not sure how to do without actually executing the DROP.

I'm a bit confused by this as executing the DROP is going to follow the
entries in pg_depend, which we could do also..  What am I missing there?

   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.
 
 I've no problem at all changing CREATE POLICY (and some other) deparse
 functions to look more at the catalog than the command. What I don't see
 is changing all of the create 

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
 table.  The input argument is an array of object addresses, so that you
 can request creation commands for multiple objects.  (It's not my
 intention to provide this functionality right away, but if somebody else
 wants to work on top of the current deparse patch, by my guest; if it
 proves simple enough we can still get it into 9.5 as part of this
 patch.)


 +1

Another possible function could be to diff two relations to produce a set
of DDL commands that could be used for schema migrations.

Also thank you very much! CREATE/ALTER support in event triggers is the
upcoming feature I am most excited about, and I am happy to see progress.

Thanks,
Ryan Pedela


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 this commitfest.  I don't have any issue tabling the rest of the
 discussion and future work based on that to a later time.

At some point I did consider the idea that the CREATE cases of the
deparse code could be usable without a parse tree.  For the specific
case of deparsing a DDL command as it's being ran, it's important to
have the parse tree: options such as IF EXISTS, OR REPLACE and others
don't live in the catalogs and so they must be obtained from the parse
tree.  I think it is possible to adjust some of the code so that it can
run with a NULL parse tree, and set the options to empty in those cases;
that should return a usable command to re-create the object.

One point to bear in mind is that there is still some work left; and if
I need to additionally add extra interfaces so that this code can be
called from outside event triggers, I will not have any time to review
other people's patches from the commitfest.  So here's my suggestion:
let this be pushed with the current interfaces only, with an eye towards
not adding obstacles to future support for a different interface.
That's more in the spirit of incremental improvement than trying to cram
everything in the initial commit.

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
table.  The input argument is an array of object addresses, so that you
can request creation commands for multiple objects.  (It's not my
intention to provide this functionality right away, but if somebody else
wants to work on top of the current deparse patch, by my guest; if it
proves simple enough we can still get it into 9.5 as part of this
patch.)

-- 
Álvaro Herrerahttp://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


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 small enough and reasonable enough change to happen
  during this commitfest.  I don't have any issue tabling the rest of the
  discussion and future work based on that to a later time.
 
 At some point I did consider the idea that the CREATE cases of the
 deparse code could be usable without a parse tree.  For the specific
 case of deparsing a DDL command as it's being ran, it's important to
 have the parse tree: options such as IF EXISTS, OR REPLACE and others
 don't live in the catalogs and so they must be obtained from the parse
 tree.

Yep.

 One point to bear in mind is that there is still some work left; and if
 I need to additionally add extra interfaces so that this code can be
 called from outside event triggers, I will not have any time to review
 other people's patches from the commitfest.  So here's my suggestion:
 let this be pushed with the current interfaces only, with an eye towards
 not adding obstacles to future support for a different interface.
 That's more in the spirit of incremental improvement than trying to cram
 everything in the initial commit.

+1

 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
 table.  The input argument is an array of object addresses, so that you
 can request creation commands for multiple objects.  (It's not my
 intention to provide this functionality right away, but if somebody else
 wants to work on top of the current deparse patch, by my guest; if it
 proves simple enough we can still get it into 9.5 as part of this
 patch.)

I think that'd be usefuful functionality, but I can't see it being
realistic for 9.5 unless somebody starts working on it right now with
full force.

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


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


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 pg_dump manages somehow.

That's not the same. pg_dump recreates a static state, not running log
of changes that can be replayed.

 I didn't specify that a single command had to be used.

Well, if you want to get 'the create table statement' you'll need to
decide what you want. With ddl event triggers it's clear - something
that, when replayed, results in the same catalog contents. Such an easy
definition doesn't, as far as I know, exist for a set of functions you
seem to imagine.

 Further, the actual deparse_CreateStmt doesn't look like it'd have a
 terribly hard time producing something without access to the
 parsetree.

The interesting bit isn't the deparse_CreateStmt itself, but all the
stuff that's also triggered when you do a CREATE TABLE
nontrival-stuff;. utility.c will first transform the statement and then
run run and stash every created subcommand. And the subcommands will
*also* get deparsed.

Just think about what to do about CREATE TABLE foo(id serial primary
key, data text, bar_id REFERENCES foo.bar); - there's no way you can get
which other objects to dump from the catalog alone. What for a schema,
considering CREATE SCHEMA ... (schema_elements)+;?

Sure, individual subcommands could refer to the catalog instead of the
parse tree. But to get the whole thing you can't easily just refer to
it.

 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.

DROP's already in 9.4, the additions in 9.5 were more or less usability
things.  The problem generating DROPs is right now more identifying
which one you want to drop and checking the dependencies - the latter
I'm not sure how to do without actually executing the DROP.

   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.

I've no problem at all changing CREATE POLICY (and some other) deparse
functions to look more at the catalog than the command. What I don't see
is changing all of the create deparse functions, guarantee that they are
usable for getting the DDL of catalog objects and provide SQL
accessible infrastructure for that. That's a *massive* undertaking.

What I mean with 'sharing some of the infrastructure' is that I can see
a good portion of the deparse_* functions being reused for what you
desire.

But the decision about which of those to call will be an entirely
separate project. So is a whole new infrastructure to consider locking
and visibility (all the deparse stuff uses continualy evolving catalog
snapshots!) that it'll need as that's a problem the event trigger stuff
has much less to care about, because the objects are new rows and thus
can't be updated by other backends.

  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.

Right. We got a *massive* program that solves dependencies and doesn't
do all that much useful/correct things if you only let it dump
individual objects. And that dumping of individual objects is what you
want... ;)

Greetings,

Andres Freund


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


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 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
From bb8b0027e82e628fb098b9707f36fa5ce08b9234 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Sat, 21 Feb 2015 16:36:34 +0100
Subject: [PATCH 3/3] Support SET/RESET in CREATE/ALTER FUNCTION.

---
 src/backend/tcop/deparse_utility.c | 61 ++
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/src/backend/tcop/deparse_utility.c b/src/backend/tcop/deparse_utility.c
index 35c4eca..cb52cb2 100644
--- a/src/backend/tcop/deparse_utility.c
+++ b/src/backend/tcop/deparse_utility.c
@@ -2802,14 +2802,46 @@ deparse_CreateDomain(Oid objectId, Node *parsetree)
 	return createDomain;
 }
 
+static ObjTree *
+deparse_FunctionSet(VariableSetKind kind, char *name, char *value)
+{
+	ObjTree	   *r;
+
+	if (kind == VAR_RESET_ALL)
+	{
+		r = new_objtree_VA(RESET ALL, 0);
+	}
+	else if (value != NULL)
+	{
+		/*
+		 * Some GUC variable names are 'LIST' type and hence must not be
+		 * quoted. FIXME: shouldn't this and pg_get_functiondef() rather use
+		 * guc.c to check for GUC_LIST?
+		 */
+		if (pg_strcasecmp(name, DateStyle) == 0
+			|| pg_strcasecmp(name, search_path) == 0)
+			r = new_objtree_VA(SET %{set_name}I TO %{set_value}s, 0);
+		else
+			r = new_objtree_VA(SET %{set_name}I TO %{set_value}L, 0);
+
+		append_string_object(r, set_name, name);
+		append_string_object(r, set_value, value);
+	}
+	else
+	{
+		r = new_objtree_VA(RESET %{set_name}I, 0);
+		append_string_object(r, set_name, name);
+	}
+
+	return r;
+}
+
 /*
  * 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.
  */
 static ObjTree *
 deparse_CreateFunction(Oid objectId, Node *parsetree)
@@ -2825,6 +2857,7 @@ deparse_CreateFunction(Oid objectId, Node *parsetree)
 	char	   *probin;
 	List	   *params;
 	List	   *defaults;
+	List	   *sets = NIL;
 	ListCell   *cell;
 	ListCell   *curdef;
 	ListCell   *table_params = NULL;
@@ -3089,7 +3122,21 @@ deparse_CreateFunction(Oid objectId, Node *parsetree)
 		append_float_object(tmp, rows, procForm-prorows);
 	append_object_object(createFunc, rows, tmp);
 
-	append_array_object(createFunc, set_options, NIL);
+	foreach(cell, node-options)
+	{
+		DefElem	*defel = (DefElem *) lfirst(cell);
+		ObjTree	   *tmp = NULL;
+
+		if (strcmp(defel-defname, set) == 0)
+		{
+			VariableSetStmt *sstmt = (VariableSetStmt *) defel-arg;
+			char *value = ExtractSetVariableArgs(sstmt);
+
+			tmp = deparse_FunctionSet(sstmt-kind, sstmt-name, value);
+			sets = lappend(sets, new_object_object(tmp));
+		}
+	}
+	append_array_object(createFunc, set_options, sets);
 
 	if (probin == NULL)
 	{
@@ -3114,8 +3161,6 @@ deparse_CreateFunction(Oid objectId, Node *parsetree)
  *
  * 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.
  */
 static ObjTree *
 deparse_AlterFunction(Oid objectId, Node *parsetree)
@@ -3203,8 +3248,12 @@ deparse_AlterFunction(Oid objectId, Node *parsetree)
 	 defGetNumeric(defel));
 		}
 		else if (strcmp(defel-defname, set) == 0)
-			elog(ERROR, unimplemented deparse of ALTER FUNCTION SET);
+		{
+			VariableSetStmt *sstmt = (VariableSetStmt *) defel-arg;
+			char *value = ExtractSetVariableArgs(sstmt);
 
+			tmp = deparse_FunctionSet(sstmt-kind, sstmt-name, value);
+		}
 		elems = lappend(elems, new_object_object(tmp));
 	}
 
-- 

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 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 alvhe...@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)
 +{


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[] =
   { 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


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


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


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 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;
 + RelationpolRel = 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;
 + boolisnull;
 +
 + storedexpr = heap_getattr(polTup, Anum_pg_policy_polqual,
 +   
 RelationGetDescr(polRel), isnull);
 + if (isnull)
 + 

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), 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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services
From b366da953a10dea2fc2ae3e172cf15ab10e1e7ad Mon Sep 17 00:00:00 2001
From: Alvaro Herrera alvhe...@alvh.no-ip.org
Date: Thu, 19 Feb 2015 17:02:41 -0300
Subject: [PATCH] Support more commands in event triggers

COMMENT, SECURITY LABEL, and GRANT/REVOKE now also fire
ddl_command_start and ddl_command_end event triggers, when they operate
on database-local objects.

Reviewed-By: Michael Paquier, Andres Freund, Stephen Frost
---
 doc/src/sgml/event-trigger.sgml  | 125 ++-
 src/backend/commands/event_trigger.c |  34 +-
 src/backend/tcop/utility.c   |  68 +++
 src/include/commands/event_trigger.h |   1 +
 4 files changed, 211 insertions(+), 17 deletions(-)

diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml
index 156c463..04353ea 100644
--- a/doc/src/sgml/event-trigger.sgml
+++ b/doc/src/sgml/event-trigger.sgml
@@ -36,7 +36,9 @@
 
para
  The literalddl_command_start/ event occurs just before the
- execution of a literalCREATE/, literalALTER/, or literalDROP/
+ execution of a literalCREATE/, literalALTER/, literalDROP/,
+ literalSECURITY LABEL/,
+ literalCOMMENT/, literalGRANT/ or literalREVOKE/
  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
@@ -123,14 +125,15 @@
 
table id=event-trigger-by-command-tag
  titleEvent Trigger Support by Command Tag/title
- tgroup cols=5
+ tgroup cols=6
   thead
row
-entrycommand tag/entry
+entryCommand Tag/entry
 entryliteralddl_command_start/literal/entry
 entryliteralddl_command_end/literal/entry
 entryliteralsql_drop/literal/entry
 entryliteraltable_rewrite/literal/entry
+entryNotes/entry
/row
   /thead
   tbody
@@ -140,6 +143,7 @@
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=center/entry
/row
row
 entry align=leftliteralALTER COLLATION/literal/entry
@@ -147,6 +151,7 @@
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=center/entry
/row
row
 entry align=leftliteralALTER CONVERSION/literal/entry
@@ -154,6 +159,7 @@
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
 entry align=centerliteral-/literal/entry
+entry align=center/entry
/row
row
 entry align=leftliteralALTER DOMAIN/literal/entry
@@ -161,6 +167,7 @@
 entry align=centerliteralX/literal/entry
 entry align=centerliteral-/literal/entry
 entry align=centerliteral-/literal/entry
+

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
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services
From 69d0b62787c2a2a514f70247ccde98c4c43d70a0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera alvhe...@alvh.no-ip.org
Date: Thu, 19 Feb 2015 14:38:09 -0300
Subject: [PATCH] Have RENAME routines return ObjectAddress rather than OID

This lets them include an objectSubId when appropriate (i.e. when
renaming relation columns and composite type attributes), and
additionally they make the catalog OID available for further processing.
The object OID that was previously returned is still available in the
objectId field of the returned ObjectAddress.

This isn't terribly exciting in itself but it supports future event
trigger changes.

Discussion: 20150218213255.gc6...@tamriel.snowman.net
Reviewed-By: Stephen Frost
---
 src/backend/catalog/dependency.c|  6 ++-
 src/backend/catalog/objectaddress.c |  6 +++
 src/backend/commands/alter.c| 14 +--
 src/backend/commands/dbcommands.c   |  7 +++-
 src/backend/commands/policy.c   |  7 +++-
 src/backend/commands/schemacmds.c   |  7 +++-
 src/backend/commands/tablecmds.c| 77 +
 src/backend/commands/tablespace.c   |  7 +++-
 src/backend/commands/trigger.c  |  7 +++-
 src/backend/commands/typecmds.c |  6 ++-
 src/backend/commands/user.c |  7 +++-
 src/backend/rewrite/rewriteDefine.c |  7 +++-
 src/include/catalog/objectaddress.h | 16 
 src/include/commands/alter.h|  3 +-
 src/include/commands/dbcommands.h   |  3 +-
 src/include/commands/policy.h   |  2 +-
 src/include/commands/schemacmds.h   |  2 +-
 src/include/commands/tablecmds.h|  9 +++--
 src/include/commands/tablespace.h   |  3 +-
 src/include/commands/trigger.h  |  3 +-
 src/include/commands/typecmds.h |  2 +-
 src/include/commands/user.h |  2 +-
 src/include/rewrite/rewriteDefine.h |  3 +-
 23 files changed, 155 insertions(+), 51 deletions(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index bacb242..25ff326 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -2269,8 +2269,9 @@ free_object_addresses(ObjectAddresses *addrs)
 ObjectClass
 getObjectClass(const ObjectAddress *object)
 {
-	/* only pg_class entries can have nonzero objectSubId */
-	if (object-classId != RelationRelationId 
+	/* only pg_class and pg_type entries can have nonzero objectSubId */
+	if ((object-classId != RelationRelationId 
+		 object-classId != TypeRelationId) 
 		object-objectSubId != 0)
 		elog(ERROR, invalid non-zero objectSubId for object class %u,
 			 object-classId);
@@ -2285,6 +2286,7 @@ getObjectClass(const ObjectAddress *object)
 			return OCLASS_PROC;
 
 		case TypeRelationId:
+			/* caller must check objectSubId */
 			return OCLASS_TYPE;
 
 		case CastRelationId:
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d899dd7..2bbc15d 100644
--- 

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.
  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 Herrerahttp://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


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 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 Herrerahttp://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


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


signature.asc
Description: Digital signature


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


signature.asc
Description: Digital signature


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
 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 Herrerahttp://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


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


signature.asc
Description: Digital signature


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


signature.asc
Description: Digital signature


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


signature.asc
Description: Digital signature


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 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 Herrerahttp://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


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