Re: New Event Trigger: table_rewrite

On 16 November 2014 06:59, Michael Paquier <michael(dot)paquier(at)gmail(dot)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;
> 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.

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