Re: [HACKERS] deparsing utility commands
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
* 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
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
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
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
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
* 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
* 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
* 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
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
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
