On Fri, Sep 26, 2014 at 6:59 AM, Alvaro Herrera
<alvhe...@2ndquadrant.com> wrote:
> Actually here's a different split of these patches, which I hope makes
> more sense. My intention here is that patches 0001 to 0004 are simple
> changes that can be pushed right away; they are not directly related to
> the return-creation-command feature. Patches 0005 to 0027 implement
> that feature incrementally. You can see in patch 0005 the DDL commands
> that are still not implemented in deparse (they are the ones that have
> an elog(ERROR) rather than a "command = NULL"). Patch 0006 adds calls
> in ProcessUtilitySlow() to each command, so that the object(s) being
> touched are added to the event trigger command stash.
>
> Patches from 0007 to 0027 (excepting patch 0017) implement one or a
> small number of commands in deparse. Patch 0017 is necessary
> infrastructure in ALTER TABLE to support deparsing that one.
>
> My intention with the later patches is that they would all be pushed as
> a single commit, i.e. the deparse support would be implemented for all
> commands in a fell swoop rather than piecemeal -- except possibly patch
> 0017 (the ALTER TABLE infrastructure). I split them up only for ease of
> review. Of course, before pushing we (I) need to implement deparsing
> for all the remaining commands.
Thanks for the update. This stuff is still on my TODO list and I was
planning to look at it at some extent today btw :) Andres has already
sent some comments (20140925235724.gh16...@awork2.anarazel.de) though,
I'll try to not overlap with what has already been mentioned.
Patch 1: I still like this patch as it gives a clear separation of the
built-in functions and the sub-functions of ruleutils.c that are
completely independent. Have you considered adding the external
declaration of quote_all_identifiers as well? It is true that this
impacts extensions (some of my stuff as well), but my point is to bite
the bullet and make the separation cleaner between builtins.h and
ruleutils.h. Attached is a patch that can be applied on top of patch 1
doing so... Feel free to discard for the potential breakage this would
create though.
Patch 2:
1) Declarations of renameatt are inconsistent:
@tablecmds.c:
-renameatt(RenameStmt *stmt)
+renameatt(RenameStmt *stmt, int32 *objsubid)
@tablecmds.h:
-extern Oid renameatt(RenameStmt *stmt);
+extern Oid renameatt(RenameStmt *stmt, int *attnum);
Looking at this code, for correctness renameatt should use everywhere
"int *attnum" and ExecRenameStmt should use "int32 *subobjid".
2) Just a smallish picky formatting remark, I would have done that on
a single line:
+ attnum =
+ renameatt_internal(relid,
3) in ExecRenameStmt, I think that you should set subobjid for
aggregate, collations, fdw, etc. There is an access to ObjectAddress
so that's easy to set using address.objectSubId.
4) This comment is misleading, as for an attribute what is returned is
actually an attribute number:
+ * Return value is the OID of the renamed object. The objectSubId, if any,
+ * is returned in objsubid.
*/
5) Perhaps it is worth mentioning at the top of renameatt that the
attribute number is returned as well?
Patch 3: Looks fine, a bit of testing is showing up that this works as expected:
=# CREATE ROLE foo;
CREATE ROLE
=# CREATE TABLE aa (a int);
CREATE TABLE
=# CREATE OR REPLACE FUNCTION abort_grant()
RETURNS event_trigger AS $$
BEGIN
RAISE NOTICE 'Event: %', tg_event;
RAISE EXCEPTION 'Execution of command % forbidden', tg_tag;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION
=# CREATE EVENT TRIGGER abort_grant_trigger ON ddl_command_start
WHEN TAG IN ('GRANT', 'REVOKE') EXECUTE PROCEDURE abort_grant();
CREATE EVENT TRIGGER
=# GRANT SELECT ON aa TO foo;
NOTICE: 00000: Event: ddl_command_start
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command GRANT forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068
=# DROP EVENT TRIGGER abort_grant_trigger;
DROP EVENT TRIGGER
=# CREATE EVENT TRIGGER abort_grant_trigger ON ddl_command_end
WHEN TAG IN ('GRANT', 'REVOKE') EXECUTE PROCEDURE abort_grant();
CREATE EVENT TRIGGER
=# REVOKE SELECT ON aa FROM foo;
NOTICE: 00000: Event: ddl_command_end
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command REVOKE forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068
Patch 4: Shouldn't int32 be used instead of uint32 in the declaration
of CommentObject? And yes, adding support for only ddl_command_start
and ddl_command_end is enough. Let's not play with dropped objects in
this area... Similarly to the test above:
=# comment on table aa is 'test';
NOTICE: 00000: Event: ddl_command_start
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command COMMENT forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068
Regards,
--
Michael
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d77b3ee..fc8dc80 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -57,6 +57,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/tqual.h"
#include "dblink.h"
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 2045774..03c6e67 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -50,6 +50,7 @@
#include "parser/parsetree.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4c49776..69d19e3 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -36,6 +36,7 @@
#include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+#include "utils/ruleutils.h"
PG_MODULE_MAGIC;
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index fdbd313..f865ad7 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -27,6 +27,7 @@
#include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/relcache.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
diff --git a/contrib/worker_spi/worker_spi.c b/contrib/worker_spi/worker_spi.c
index 328c722..1502b61 100644
--- a/contrib/worker_spi/worker_spi.c
+++ b/contrib/worker_spi/worker_spi.c
@@ -38,6 +38,7 @@
#include "lib/stringinfo.h"
#include "pgstat.h"
#include "utils/builtins.h"
+#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
#include "tcop/utility.h"
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 5eb8fd4..dd1b633 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -53,6 +53,7 @@
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index b69b75b..249923b 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -74,6 +74,7 @@
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 9a0afa4..1475934 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -52,6 +52,7 @@
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
#include "utils/tqual.h"
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index d1c8bb0..c6e69d9 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -36,6 +36,7 @@
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 9bf0098..a932ddd 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -53,6 +53,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 5d8a001..5e8001a 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -43,6 +43,7 @@
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 7c1939f..3578070 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -58,6 +58,7 @@
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index e1763a3..4f262fa 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -23,6 +23,7 @@
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/numeric.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
#include "mb/pg_wchar.h"
diff --git a/src/backend/utils/adt/quote.c b/src/backend/utils/adt/quote.c
index d1336b4..6208c93 100644
--- a/src/backend/utils/adt/quote.c
+++ b/src/backend/utils/adt/quote.c
@@ -14,6 +14,7 @@
#include "postgres.h"
#include "utils/builtins.h"
+#include "utils/ruleutils.h"
/*
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index c0314ee..a7c618e 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -38,6 +38,7 @@
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index c0156fa..6b739ca 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -50,6 +50,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/rel.h"
+#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index c3171b5..8b27786 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -30,6 +30,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_locale.h"
+#include "utils/ruleutils.h"
#include "utils/sortsupport.h"
diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 5ff1461..43a7502 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -45,6 +45,7 @@
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+#include "utils/ruleutils.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d208314..c81ca2c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -79,6 +79,7 @@
#include "utils/plancache.h"
#include "utils/portal.h"
#include "utils/ps_status.h"
+#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
#include "utils/tzparser.h"
#include "utils/xml.h"
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index fb1b4a4..0ef0323 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -666,7 +666,6 @@ extern Datum record_image_ge(PG_FUNCTION_ARGS);
extern Datum btrecordimagecmp(PG_FUNCTION_ARGS);
/* ruleutils.c */
-extern bool quote_all_identifiers;
extern Datum pg_get_ruledef(PG_FUNCTION_ARGS);
extern Datum pg_get_ruledef_ext(PG_FUNCTION_ARGS);
extern Datum pg_get_viewdef(PG_FUNCTION_ARGS);
@@ -689,10 +688,6 @@ extern Datum pg_get_function_arguments(PG_FUNCTION_ARGS);
extern Datum pg_get_function_identity_arguments(PG_FUNCTION_ARGS);
extern Datum pg_get_function_result(PG_FUNCTION_ARGS);
extern Datum pg_get_function_arg_default(PG_FUNCTION_ARGS);
-extern const char *quote_identifier(const char *ident);
-extern char *quote_qualified_identifier(const char *qualifier,
- const char *ident);
-
/* tid.c */
extern Datum tidin(PG_FUNCTION_ARGS);
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index 520b066..4a3fb77 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -17,6 +17,10 @@
#include "nodes/parsenodes.h"
#include "nodes/pg_list.h"
+extern bool quote_all_identifiers;
+extern const char *quote_identifier(const char *ident);
+extern char *quote_qualified_identifier(const char *qualifier,
+ const char *ident);
extern char *pg_get_indexdef_string(Oid indexrelid);
extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 893f3a4..06b9b88 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -22,6 +22,7 @@
#include "parser/scanner.h"
#include "parser/scansup.h"
#include "utils/builtins.h"
+#include "utils/ruleutils.h"
/* Location tracking support --- simpler than bison's default */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
