Re: [HACKERS] New Event Trigger: table_rewrite
On Thu, Nov 20, 2014 at 10:37 PM, Dimitri Fontaine
dimi...@2ndquadrant.fr wrote:
Alvaro Herrera alvhe...@2ndquadrant.com writes:
CLUSTER and VACUUM are not part of the supported commands anymore, so
I think that we could replace that by the addition of a reference
number in the cell of ALTER TABLE for the event table_rewrite and
write at the bottom of the table a description of how this event
behaves with ALTER TABLE. Note as well that might or might not is
not really helpful for the user.
That's precisely why we have an event trigger here, I think --- for some
subcommands, it's not easy to determine whether a rewrite happens or
not. (I think SET TYPE is the one). I don't think we want to document
precisely under what condition a rewrite takes place.
Yeah, the current documentation expands to the following sentence, as
browsed in
http://www.postgresql.org/docs/9.3/interactive/sql-altertable.html
As an exception, if the USING clause does not change the column
contents and the old type is either binary coercible to the new type
or an unconstrained domain over the new type, a table rewrite is not
needed, but any indexes on the affected columns must still be rebuilt.
I don't think that might or might not is less helpful in the context
of the Event Trigger, because the whole point is that the event is only
triggered in case of a rewrite. Of course we could cross link the two
paragraphs or something.
2) The examples of SQL queries provided are still in lower case in the
docs, that's contrary to the rest of the docs where upper case is used
for reserved keywords.
Right, being consistent trumps personal preferences, changed in the
attached.
Yes please. nitpick Another thing in that sample code is not current_hour
between 1 and 6. That reads strange to me. It should be equally
correct to spell it as current_hour not between 1 and 6, which seems
more natural. /
True, fixed in the attached.
The status of this patch was not updated on the commit fest app, so I
lost track of it. Sorry for not answering earlier btw.
The following things to note about v5:
1) There are still mentions of VACUUM, ANALYZE and CLUSTER:
@@ -264,6 +275,10 @@ check_ddl_tag(const char *tag)
obtypename = tag + 6;
else if (pg_strncasecmp(tag, DROP , 5) == 0)
obtypename = tag + 5;
+ else if (pg_strncasecmp(tag, ANALYZE, 7) == 0 ||
+pg_strncasecmp(tag, CLUSTER, 7) == 0 ||
+pg_strncasecmp(tag, VACUUM, 6) == 0)
+ return EVENT_TRIGGER_COMMAND_TAG_OK;
2) There are a couple of typos and incorrect styling, like if(. Nothing huge..
Cleanup is done in the attached.
In any case, all the issues mentioned seem to have been addressed, so
switching this patch to ready for committer.
Regards,
--
Michael
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 6a3002f..17b0818 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -42,11 +42,14 @@
#include utils/syscache.h
#include tcop/utility.h
-
+/*
+ * Data Structure for sql_drop and table_rewrite Event Trigger support.
+ */
typedef struct EventTriggerQueryState
{
slist_head SQLDropList;
bool in_sql_drop;
+ Oid table_rewrite_oid;
MemoryContext cxt;
struct EventTriggerQueryState *previous;
} EventTriggerQueryState;
@@ -119,11 +122,14 @@ static void AlterEventTriggerOwner_internal(Relation rel,
HeapTuple tup,
Oid newOwnerId);
static event_trigger_command_tag_check_result check_ddl_tag(const char *tag);
+static event_trigger_command_tag_check_result check_table_rewrite_ddl_tag(
+ const char *tag);
static void error_duplicate_filter_variable(const char *defname);
static Datum filter_list_to_array(List *filterlist);
static Oid insert_event_trigger_tuple(char *trigname, char *eventname,
Oid evtOwner, Oid funcoid, List *tags);
static void validate_ddl_tags(const char *filtervar, List *taglist);
+static void validate_table_rewrite_tags(const char *filtervar, List *taglist);
static void EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata);
/*
@@ -154,7 +160,8 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
/* Validate event name. */
if (strcmp(stmt-eventname, ddl_command_start) != 0
strcmp(stmt-eventname, ddl_command_end) != 0
- strcmp(stmt-eventname, sql_drop) != 0)
+ strcmp(stmt-eventname, sql_drop) != 0
+ strcmp(stmt-eventname, table_rewrite) != 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg(unrecognized event name \%s\,
@@ -183,6 +190,9 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
strcmp(stmt-eventname, sql_drop) == 0)
tags != NULL)
validate_ddl_tags(tag, tags);
+ else if (strcmp(stmt-eventname, table_rewrite) == 0
+ tags != NULL)
+ validate_table_rewrite_tags(tag, tags);
/*
* Give user a nice error message if an event trigger of the same name
Re: [HACKERS] New Event Trigger: table_rewrite
Alvaro Herrera alvhe...@2ndquadrant.com writes: CLUSTER and VACUUM are not part of the supported commands anymore, so I think that we could replace that by the addition of a reference number in the cell of ALTER TABLE for the event table_rewrite and write at the bottom of the table a description of how this event behaves with ALTER TABLE. Note as well that might or might not is not really helpful for the user. That's precisely why we have an event trigger here, I think --- for some subcommands, it's not easy to determine whether a rewrite happens or not. (I think SET TYPE is the one). I don't think we want to document precisely under what condition a rewrite takes place. Yeah, the current documentation expands to the following sentence, as browsed in http://www.postgresql.org/docs/9.3/interactive/sql-altertable.html As an exception, if the USING clause does not change the column contents and the old type is either binary coercible to the new type or an unconstrained domain over the new type, a table rewrite is not needed, but any indexes on the affected columns must still be rebuilt. I don't think that “might or might not” is less helpful in the context of the Event Trigger, because the whole point is that the event is only triggered in case of a rewrite. Of course we could cross link the two paragraphs or something. 2) The examples of SQL queries provided are still in lower case in the docs, that's contrary to the rest of the docs where upper case is used for reserved keywords. Right, being consistent trumps personal preferences, changed in the attached. Yes please. nitpick Another thing in that sample code is not current_hour between 1 and 6. That reads strange to me. It should be equally correct to spell it as current_hour not between 1 and 6, which seems more natural. / True, fixed in the attached. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index 6f71a27..0a80993 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -65,6 +65,16 @@ /para para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal. While other +control statements are available to rewrite a table, +like literalCLUSTER/literal and literalVACUUM/literal, +the literaltable_rewrite/ event is currently only triggered by +the literalALTER TABLE/literal command, which might or might not need +to rewrite the table. + /para + + para Event triggers (like other functions) cannot be executed in an aborted transaction. Thus, if a DDL command fails with an error, any associated literalddl_command_end/ triggers will not be executed. Conversely, @@ -120,6 +130,7 @@ entryliteralddl_command_start/literal/entry entryliteralddl_command_end/literal/entry entryliteralsql_drop/literal/entry +entryliteraltable_rewrite/literal/entry /row /thead tbody @@ -128,510 +139,595 @@ entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER COLLATION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER CONVERSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER DOMAIN/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER EXTENSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN DATA WRAPPER/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN TABLE/literal/entry entry align=centerliteralX/literal/entry entry
Re: [HACKERS] New Event Trigger: table_rewrite
Alvaro Herrera alvhe...@2ndquadrant.com writes: Almost the whole of that function is conditions to bail out clustering the relation if things have changed since the relation list was collected. It seems wrong to invoke the event trigger in all those cases; it's going to fire spuriously. I think you should move the invocation of the event trigger at the end, just before rebuild_relation is called. Not sure where relative to the predicate lock stuff therein; probably before, so that we avoid doing that dance if the event trigger function decides to jump ship. Actually when you do a CLUSTER or a VACUUM FULL you know that the table is going to be rewritten on disk, because that's about the only purpose of the command. Given the complexity involved here, the new version of the patch (attached) has removed support for those statements. In ATRewriteTables, it seems wrong to call it after make_new_heap. If the event trigger function aborts, we end up with useless work done there; so I think it should be called before that. Also, why do you have the evt_table_rewrite_fired stuff? I think you should fire one event per table, no? Fixed in the attached version of the patch. The second ATRewriteTable call in ATRewriteTables does not actually rewrite the table; it only scans it to verify constraints. So I'm thinking you shouldn't call this event trigger there. Or, if we decide we want this, we probably also need something for the table scans in ALTER DOMAIN too. Fixed in the attached version of the patch. You still have the ANALYZE thing in docs, which now should be removed. Fixed in the attached version of the patch. -- Dimitri Fontaine06 63 07 10 78 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index 6f71a27..78ec27b 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -65,6 +65,16 @@ /para para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal. While other +control statements are available to rewrite a table, +like literalCLUSTER/literal and literalVACUUM/literal, +the literaltable_rewrite/ event is currently only triggered by +the literalALTER TABLE/literal command, which might or might not need +to rewrite the table. + /para + + para Event triggers (like other functions) cannot be executed in an aborted transaction. Thus, if a DDL command fails with an error, any associated literalddl_command_end/ triggers will not be executed. Conversely, @@ -120,6 +130,7 @@ entryliteralddl_command_start/literal/entry entryliteralddl_command_end/literal/entry entryliteralsql_drop/literal/entry +entryliteraltable_rewrite/literal/entry /row /thead tbody @@ -128,510 +139,595 @@ entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER COLLATION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER CONVERSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER DOMAIN/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER EXTENSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN DATA WRAPPER/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN TABLE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry +entry align=centerliteral-/literal/entry /row row entry
Re: [HACKERS] New Event Trigger: table_rewrite
On Tue, Nov 18, 2014 at 5:14 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@gmail.com writes: It seems pretty weird, also, that the event trigger will fire after we've taken AccessExclusiveLock when you cluster a particular relation, and before we've taken AccessExclusiveLock when you cluster database-wide. That's more or less an implementation artifact of the current code that we're exposing to the use for, really, no good reason. In the CLUSTER implementation we have only one call site for invoking the Event Trigger, in cluster_rel(). While it's true that in the single relation case, the relation is opened in cluster() then cluster_rel() is called, the opening is done with NoLock in cluster(): rel = heap_open(tableOid, NoLock); My understanding is that the relation locking only happens in cluster_rel() at this line: OldHeap = try_relation_open(tableOid, AccessExclusiveLock); Please help me through the cluster locking strategy here, I feel like I'm missing something obvious, as my conclusion from re-reading the code in lights of your comment is that your comment is not accurate with respect to the current state of the code. Unless I'm missing something, when you cluster a particular relation, cluster() does this: /* Find and lock the table */ rel = heap_openrv(stmt-relation, AccessExclusiveLock); I don't see the rel = heap_open(tableOid, NoLock); line you quoted anywhere. -- 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] New Event Trigger: table_rewrite
On Tue, Nov 18, 2014 at 5:34 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Almost the whole of that function is conditions to bail out clustering the relation if things have changed since the relation list was collected. It seems wrong to invoke the event trigger in all those cases; it's going to fire spuriously. I think you should move the invocation of the event trigger at the end, just before rebuild_relation is called. Not sure where relative to the predicate lock stuff therein; probably before, so that we avoid doing that dance if the event trigger function decides to jump ship. I can see two problems with that: 1. What if the conditions aren't true any more after the event trigger has run? Then it's unsafe. 2. If we do it that way, then we'll unnecessarily wait for a lock on the relation even if the event trigger is just going to bail out. -- 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] New Event Trigger: table_rewrite
On Wed, Nov 19, 2014 at 1:01 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Nov 18, 2014 at 5:14 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@gmail.com writes: It seems pretty weird, also, that the event trigger will fire after we've taken AccessExclusiveLock when you cluster a particular relation, and before we've taken AccessExclusiveLock when you cluster database-wide. That's more or less an implementation artifact of the current code that we're exposing to the use for, really, no good reason. In the CLUSTER implementation we have only one call site for invoking the Event Trigger, in cluster_rel(). While it's true that in the single relation case, the relation is opened in cluster() then cluster_rel() is called, the opening is done with NoLock in cluster(): rel = heap_open(tableOid, NoLock); My understanding is that the relation locking only happens in cluster_rel() at this line: OldHeap = try_relation_open(tableOid, AccessExclusiveLock); Please help me through the cluster locking strategy here, I feel like I'm missing something obvious, as my conclusion from re-reading the code in lights of your comment is that your comment is not accurate with respect to the current state of the code. Unless I'm missing something, when you cluster a particular relation, cluster() does this: /* Find and lock the table */ rel = heap_openrv(stmt-relation, AccessExclusiveLock); I don't see the rel = heap_open(tableOid, NoLock); line you quoted anywhere. ...which is because I have the 9.1 branch checked out. Genius. But what I said originally is still true, because the current code looks like this: /* Find, lock, and check permissions on the table */ tableOid = RangeVarGetRelidExtended(stmt-relation, AccessExclusiveLock, false, false, RangeVarCallbackOwnsTable, NULL); rel = heap_open(tableOid, NoLock); It's true that the heap_open() is not acquiring any lock. But the RangeVarGetRelidExtended() call right before it is. -- 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] New Event Trigger: table_rewrite
On Thu, Nov 20, 2014 at 2:57 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Fixed in the attached version of the patch. Thanks! Things are moving nicely for this patch. Patch compiles and passes check-world. Some minor comments about the latest version: 1) Couldn't this paragraph be reworked? para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal. While other +control statements are available to rewrite a table, +like literalCLUSTER/literal and literalVACUUM/literal, +the literaltable_rewrite/ event is currently only triggered by +the literalALTER TABLE/literal command, which might or might not need +to rewrite the table. + /para CLUSTER and VACUUM are not part of the supported commands anymore, so I think that we could replace that by the addition of a reference number in the cell of ALTER TABLE for the event table_rewrite and write at the bottom of the table a description of how this event behaves with ALTER TABLE. Note as well that might or might not is not really helpful for the user. 2) The examples of SQL queries provided are still in lower case in the docs, that's contrary to the rest of the docs where upper case is used for reserved keywords. + para +Here's an example implementing such a policy. +programlisting +create or replace function no_rewrite() + returns event_trigger + language plpgsql as [...] 3) This reference can be completely removed: /* * Otherwise, command should be CREATE, ALTER, or DROP. +* Or one of ANALYZE, CLUSTER, VACUUM. */ 4) In those places as well CLUSTER and VACUUM should be removed: + else if (pg_strncasecmp(tag, ANALYZE, 7) == 0 || +pg_strncasecmp(tag, CLUSTER, 7) == 0 || +pg_strncasecmp(tag, VACUUM, 6) == 0) + return EVENT_TRIGGER_COMMAND_TAG_OK; else And here: + if (pg_strcasecmp(tag, ALTER TABLE) == 0 || + pg_strcasecmp(tag, CLUSTER) == 0 || + pg_strcasecmp(tag, VACUUM) == 0 || + pg_strcasecmp(tag, ANALYZE) == 0 ) + return EVENT_TRIGGER_COMMAND_TAG_OK I am noticing that the points raised by Alvaro previously are fixed. Regards, -- Michael -- 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] New Event Trigger: table_rewrite
Michael Paquier wrote: 1) Couldn't this paragraph be reworked? para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal. While other +control statements are available to rewrite a table, +like literalCLUSTER/literal and literalVACUUM/literal, +the literaltable_rewrite/ event is currently only triggered by +the literalALTER TABLE/literal command, which might or might not need +to rewrite the table. + /para CLUSTER and VACUUM are not part of the supported commands anymore, so I think that we could replace that by the addition of a reference number in the cell of ALTER TABLE for the event table_rewrite and write at the bottom of the table a description of how this event behaves with ALTER TABLE. Note as well that might or might not is not really helpful for the user. That's precisely why we have an event trigger here, I think --- for some subcommands, it's not easy to determine whether a rewrite happens or not. (I think SET TYPE is the one). I don't think we want to document precisely under what condition a rewrite takes place. 2) The examples of SQL queries provided are still in lower case in the docs, that's contrary to the rest of the docs where upper case is used for reserved keywords. + para +Here's an example implementing such a policy. +programlisting +create or replace function no_rewrite() + returns event_trigger + language plpgsql as Yes please. nitpick Another thing in that sample code is not current_hour between 1 and 6. That reads strange to me. It should be equally correct to spell it as current_hour not between 1 and 6, which seems more natural. / -- Álvaro Herrerahttp://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] New Event Trigger: table_rewrite
Hi, Michael Paquier michael.paqu...@gmail.com writes: 1) This patch is authorizing VACUUM and CLUSTER to use the event triggers ddl_command_start and ddl_command_end, but aren't those commands actually not DDLs but control commands? Reverted in the attached version 3 of the patch. 6) in_table_rewrite seems unnecessary. Removed in the attached version 3 of the patch. On Sun, Nov 16, 2014 at 5:51 AM, Simon Riggs si...@2ndquadrant.com wrote: 4) pg_event_trigger_table_rewrite_oid is able to return only one OID, which is the one of the table being rewritten, and it is limited to one OID because VACUUM, CLUSTER and ALTER TABLE can only run on one object at the same time in a single transaction. What about thinking that we may have in the future multiple objects rewritten in a single transaction, hence multiple OIDs could be fetched? Why would this API support something which the normal trigger API doesn't, just in case we support a feature that hadn't ever been proposed or discussed? Why can't such a change wait until that feature arrives? Agreed, unchanged in the attached. Robert Haas robertmh...@gmail.com writes: It seems pretty weird, also, that the event trigger will fire after we've taken AccessExclusiveLock when you cluster a particular relation, and before we've taken AccessExclusiveLock when you cluster database-wide. That's more or less an implementation artifact of the current code that we're exposing to the use for, really, no good reason. In the CLUSTER implementation we have only one call site for invoking the Event Trigger, in cluster_rel(). While it's true that in the single relation case, the relation is opened in cluster() then cluster_rel() is called, the opening is done with NoLock in cluster(): rel = heap_open(tableOid, NoLock); My understanding is that the relation locking only happens in cluster_rel() at this line: OldHeap = try_relation_open(tableOid, AccessExclusiveLock); Please help me through the cluster locking strategy here, I feel like I'm missing something obvious, as my conclusion from re-reading the code in lights of your comment is that your comment is not accurate with respect to the current state of the code. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index 6f71a27..704a377 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -65,6 +65,12 @@ /para para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal, +literalCLUSTER/literal or literalVACUUM/literal. + /para + + para Event triggers (like other functions) cannot be executed in an aborted transaction. Thus, if a DDL command fails with an error, any associated literalddl_command_end/ triggers will not be executed. Conversely, @@ -120,518 +126,625 @@ entryliteralddl_command_start/literal/entry entryliteralddl_command_end/literal/entry entryliteralsql_drop/literal/entry +entryliteraltable_rewrite/literal/entry /row /thead tbody row +entry align=leftliteralANALYZE/literal/entry +entry align=centerliteralX/literal/entry +entry align=centerliteralX/literal/entry +entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry + /row + row entry align=leftliteralALTER AGGREGATE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER COLLATION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER CONVERSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER DOMAIN/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER EXTENSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row
Re: [HACKERS] New Event Trigger: table_rewrite
Dimitri Fontaine wrote: In the CLUSTER implementation we have only one call site for invoking the Event Trigger, in cluster_rel(). While it's true that in the single relation case, the relation is opened in cluster() then cluster_rel() is called, the opening is done with NoLock in cluster(): rel = heap_open(tableOid, NoLock); My understanding is that the relation locking only happens in cluster_rel() at this line: OldHeap = try_relation_open(tableOid, AccessExclusiveLock); Please help me through the cluster locking strategy here, I feel like I'm missing something obvious, as my conclusion from re-reading the code in lights of your comment is that your comment is not accurate with respect to the current state of the code. Almost the whole of that function is conditions to bail out clustering the relation if things have changed since the relation list was collected. It seems wrong to invoke the event trigger in all those cases; it's going to fire spuriously. I think you should move the invocation of the event trigger at the end, just before rebuild_relation is called. Not sure where relative to the predicate lock stuff therein; probably before, so that we avoid doing that dance if the event trigger function decides to jump ship. In ATRewriteTables, it seems wrong to call it after make_new_heap. If the event trigger function aborts, we end up with useless work done there; so I think it should be called before that. Also, why do you have the evt_table_rewrite_fired stuff? I think you should fire one event per table, no? The second ATRewriteTable call in ATRewriteTables does not actually rewrite the table; it only scans it to verify constraints. So I'm thinking you shouldn't call this event trigger there. Or, if we decide we want this, we probably also need something for the table scans in ALTER DOMAIN too. You still have the ANALYZE thing in docs, which now should be removed. -- Álvaro Herrerahttp://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] New Event Trigger: table_rewrite
On Sun, Nov 16, 2014 at 5:51 AM, Simon Riggs si...@2ndquadrant.com wrote: On 16 November 2014 06:59, Michael Paquier michael.paqu...@gmail.com wrote: 1) This patch is authorizing VACUUM and CLUSTER to use the event triggers ddl_command_start and ddl_command_end, but aren't those commands actually not DDLs but control commands? I could go either way on that. I'm happy to remove those from this commit. Yeah, this patch definitely shouldn't change the set of commands to which existing event triggers apply as a side-effect. There's no reason new DDL commands need to apply to the same set of operations as existing DDL commands, but the existing ones shouldn't be changed without specific discussion and agreement. It seems pretty weird, also, that the event trigger will fire after we've taken AccessExclusiveLock when you cluster a particular relation, and before we've taken AccessExclusiveLock when you cluster database-wide. That's more or less an implementation artifact of the current code that we're exposing to the use for, really, no good reason. -- 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] New Event Trigger: table_rewrite
On 16 November 2014 06:59, Michael Paquier michael.paqu...@gmail.com wrote: Note that this patch has been submitted but there have been no real discussion around it.. This seems a bit too fast to commit it, no? Committing uncontentious patches at the end of the commitfest seems normal, no? -- Simon Riggs 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] New Event Trigger: table_rewrite
On 16 November 2014 06:59, Michael Paquier michael.paqu...@gmail.com wrote:
1) This patch is authorizing VACUUM and CLUSTER to use the event
triggers ddl_command_start and ddl_command_end, but aren't those
commands actually not DDLs but control commands?
I could go either way on that. I'm happy to remove those from this commit.
2) The documentation of src/sgml/event-trigger.sgml can be improved,
particularly I think that the example function should use a maximum of
upper-case letters for reserved keywords, and also this bit:
you're not allowed to rewrite the table foo
should be rewritten to something like that:
Rewrite of table foo not allowed
3) A typo, missing a plural:
provides two built-in event trigger helper functionS
I thought the documentation was very good, in comparison to most other
feature submissions. Given that this is one of the areas I moan about
a lot, that says something.
4) pg_event_trigger_table_rewrite_oid is able to return only one OID,
which is the one of the table being rewritten, and it is limited to
one OID because VACUUM, CLUSTER and ALTER TABLE can only run on one
object at the same time in a single transaction. What about thinking
that we may have in the future multiple objects rewritten in a single
transaction, hence multiple OIDs could be fetched?
Why would this API support something which the normal trigger API
doesn't, just in case we support a feature that hadn't ever been
proposed or discussed? Why can't such a change wait until that feature
arrives?
5) parsetree is passed to cluster_rel only for
EventTriggerTableRewrite. I am not sure if there are any extension
using cluster_rel as is but wouldn't it make more sense to call
EventTriggerTableRewrite before the calls to cluster_rel instead? ISTM
that this patch is breaking cluster_rel way of doing things.
I will remove the call to CLUSTER and VACUUM as proposed above.
6) in_table_rewrite seems unnecessary.
typedef struct EventTriggerQueryState
{
slist_head SQLDropList;
boolin_sql_drop;
+ boolin_table_rewrite;
+ Oid tableOid;
We could simplify that by renaming tableOid to rewriteTableOid or
rewriteObjOid and check if its value is InvalidOid to determine if the
event table_rewrite is in use or not. Each code path setting those
variables sets them all the time similarly:
+ state-in_table_rewrite = false;
+ state-tableOid = InvalidOid;
And if tableOid is InvaliOid, in_table_rewrite is false. If it is a
valid Oid, in_table_rewrite is set to true.
Well, that seems a minor change. I'm happy to accept the original
coding, but also happy to receive suggested changes.
7) table_rewrite is kicked in ALTER TABLE only when ATRewriteTables is
used. The list of commands that actually go through this code path
should be clarified in the documentation IMO to help the user
apprehend this function.
That is somewhat orthogonal to the patch. The rules for rewriting are
quite complex, which is why this is needed and why documentation isn't
really the answer. Separate doc patch on that would be welcome.
--
Simon Riggs 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] New Event Trigger: table_rewrite
On 7 November 2014 12:35, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Simon Riggs si...@2ndquadrant.com writes: It would be more useful to work on the applications of this 1. INSERT into a table * Action start time * Schema * Tablename * Number of blocks in table which would then allow you to do these things run an assessment report showing which tables would be rewritten. That should be done by the user, from within his Event Trigger code. For that to be possible, the previous patch was missing a way to expose the OID of the table being rewritten, I've now added support for that. 2. Get access to number of blocks, so you could limit rewrites only to smaller tables by putting a block limit in place. Also, I did expand the docs to fully cover your practical use case of a table_rewrite Event Trigger implementing such a table rewrite policy. That looks complete, very useful and well documented. I'm looking to commit this tomorrow. -- Simon Riggs 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] New Event Trigger: table_rewrite
On Sun, Nov 16, 2014 at 8:57 AM, Simon Riggs si...@2ndquadrant.com wrote:
On 7 November 2014 12:35, Dimitri Fontaine dimi...@2ndquadrant.fr wrote:
Simon Riggs si...@2ndquadrant.com writes:
It would be more useful to work on the applications of this
1. INSERT into a table
* Action start time
* Schema
* Tablename
* Number of blocks in table
which would then allow you to do these things run an assessment report
showing which tables would be rewritten.
That should be done by the user, from within his Event Trigger code. For
that to be possible, the previous patch was missing a way to expose the
OID of the table being rewritten, I've now added support for that.
2. Get access to number of blocks, so you could limit rewrites only to
smaller tables by putting a block limit in place.
Also, I did expand the docs to fully cover your practical use case of a
table_rewrite Event Trigger implementing such a table rewrite policy.
That looks complete, very useful and well documented.
I'm looking to commit this tomorrow.
Patch applies, with many hunks though. Patch and documentation compile
without warnings, passing make check-world.
Some comments:
1) This patch is authorizing VACUUM and CLUSTER to use the event
triggers ddl_command_start and ddl_command_end, but aren't those
commands actually not DDLs but control commands?
2) The documentation of src/sgml/event-trigger.sgml can be improved,
particularly I think that the example function should use a maximum of
upper-case letters for reserved keywords, and also this bit:
you're not allowed to rewrite the table foo
should be rewritten to something like that:
Rewrite of table foo not allowed
3) A typo, missing a plural:
provides two built-in event trigger helper functionS
4) pg_event_trigger_table_rewrite_oid is able to return only one OID,
which is the one of the table being rewritten, and it is limited to
one OID because VACUUM, CLUSTER and ALTER TABLE can only run on one
object at the same time in a single transaction. What about thinking
that we may have in the future multiple objects rewritten in a single
transaction, hence multiple OIDs could be fetched?
5) parsetree is passed to cluster_rel only for
EventTriggerTableRewrite. I am not sure if there are any extension
using cluster_rel as is but wouldn't it make more sense to call
EventTriggerTableRewrite before the calls to cluster_rel instead? ISTM
that this patch is breaking cluster_rel way of doing things.
6) in_table_rewrite seems unnecessary.
typedef struct EventTriggerQueryState
{
slist_head SQLDropList;
boolin_sql_drop;
+ boolin_table_rewrite;
+ Oid tableOid;
We could simplify that by renaming tableOid to rewriteTableOid or
rewriteObjOid and check if its value is InvalidOid to determine if the
event table_rewrite is in use or not. Each code path setting those
variables sets them all the time similarly:
+ state-in_table_rewrite = false;
+ state-tableOid = InvalidOid;
And if tableOid is InvaliOid, in_table_rewrite is false. If it is a
valid Oid, in_table_rewrite is set to true.
7) table_rewrite is kicked in ALTER TABLE only when ATRewriteTables is
used. The list of commands that actually go through this code path
should be clarified in the documentation IMO to help the user
apprehend this function.
Note that this patch has been submitted but there have been no real
discussion around it.. This seems a bit too fast to commit it, no?
Regards,
--
Michael
--
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] New Event Trigger: table_rewrite
Simon Riggs si...@2ndquadrant.com writes: It would be more useful to work on the applications of this 1. INSERT into a table * Action start time * Schema * Tablename * Number of blocks in table which would then allow you to do these things run an assessment report showing which tables would be rewritten. That should be done by the user, from within his Event Trigger code. For that to be possible, the previous patch was missing a way to expose the OID of the table being rewritten, I've now added support for that. 2. Get access to number of blocks, so you could limit rewrites only to smaller tables by putting a block limit in place. Also, I did expand the docs to fully cover your practical use case of a table_rewrite Event Trigger implementing such a table rewrite policy. 3. It might be even cooler to contemplate having pg_stat_activity publish an estimated end time. We'd probably need some kind of time_per_block parameter for each tablespace so we can estimate the time. That feels like another patch entirely. Regards, -- Dimitri Fontaine06 63 07 10 78 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index 6f71a27..7eb3225 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -65,6 +65,12 @@ /para para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal, +literalCLUSTER/literal or literalVACUUM/literal. + /para + + para Event triggers (like other functions) cannot be executed in an aborted transaction. Thus, if a DDL command fails with an error, any associated literalddl_command_end/ triggers will not be executed. Conversely, @@ -120,518 +126,625 @@ entryliteralddl_command_start/literal/entry entryliteralddl_command_end/literal/entry entryliteralsql_drop/literal/entry +entryliteraltable_rewrite/literal/entry /row /thead tbody row +entry align=leftliteralANALYZE/literal/entry +entry align=centerliteralX/literal/entry +entry align=centerliteralX/literal/entry +entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry + /row + row entry align=leftliteralALTER AGGREGATE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER COLLATION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER CONVERSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER DOMAIN/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER EXTENSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN DATA WRAPPER/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN TABLE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FUNCTION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER LANGUAGE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry
Re: [HACKERS] New Event Trigger: table_rewrite
On 16 October 2014 10:18, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Dimitri Fontaine dimi...@2ndquadrant.fr writes: Please find attached to this email a patch to implement a new Event Trigger, fired on the the table_rewrite event. As attached, it's meant as a discussion enabler and only supports ALTER TABLE (and maybe not in all forms of it). It will need to grow support for VACUUM FULL and CLUSTER and more before getting commited. And here's already a new version of it, including support for ALTER TABLE, VACUUM and CLUSTER commands, and documentation. The patch itself looks fine overall. Docs look in place, tests OK. API changes may need more thought. I'm not sure myself, they just look fairly quick. It would be more useful to work on the applications of this 1. INSERT into a table * Action start time * Schema * Tablename * Number of blocks in table which would then allow you to do these things run an assessment report showing which tables would be rewritten. 2. Get access to number of blocks, so you could limit rewrites only to smaller tables by putting a block limit in place. 3. It might be even cooler to contemplate having pg_stat_activity publish an estimated end time. We'd probably need some kind of time_per_block parameter for each tablespace so we can estimate the time. Doing 1 and 2 at least would make this a good feature. We can do a later patch for 3, or similar, once this is accepted. -- Simon Riggs 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] New Event Trigger: table_rewrite
Dimitri Fontaine dimi...@2ndquadrant.fr writes: Please find attached to this email a patch to implement a new Event Trigger, fired on the the table_rewrite event. As attached, it's meant as a discussion enabler and only supports ALTER TABLE (and maybe not in all forms of it). It will need to grow support for VACUUM FULL and CLUSTER and more before getting commited. And here's already a new version of it, including support for ALTER TABLE, VACUUM and CLUSTER commands, and documentation. Still is a small patch: doc/src/sgml/event-trigger.sgml | 106 src/backend/commands/cluster.c | 14 ++- src/backend/commands/event_trigger.c| 106 +++- src/backend/commands/tablecmds.c| 53 -- src/backend/commands/vacuum.c | 3 +- src/backend/utils/cache/evtcache.c | 2 + src/include/commands/cluster.h | 4 +- src/include/commands/event_trigger.h| 1 + src/include/utils/evtcache.h| 3 +- src/test/regress/expected/event_trigger.out | 23 + src/test/regress/sql/event_trigger.sql | 24 + 11 files changed, 322 insertions(+), 17 deletions(-) -- Dimitri Fontaine06 63 07 10 78 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index 6f71a27..08ae838 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -65,6 +65,12 @@ /para para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal, +literalCLUSTER/literal or literalVACUUM/literal. + /para + + para Event triggers (like other functions) cannot be executed in an aborted transaction. Thus, if a DDL command fails with an error, any associated literalddl_command_end/ triggers will not be executed. Conversely, @@ -120,6 +126,7 @@ entryliteralddl_command_start/literal/entry entryliteralddl_command_end/literal/entry entryliteralsql_drop/literal/entry +entryliteraltable_rewrite/literal/entry /row /thead tbody @@ -128,510 +135,609 @@ entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER COLLATION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER CONVERSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER DOMAIN/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER EXTENSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN DATA WRAPPER/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN TABLE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FUNCTION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER LANGUAGE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER OPERATOR/literal/entry entry align=centerliteralX/literal/entry entry
