Re: New Event Trigger: table_rewrite

On Sun, Nov 16, 2014 at 8:57 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 7 November 2014 12:35, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Simon Riggs <simon(at)2ndQuadrant(dot)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;
bool in_sql_drop;
+ bool in_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