Re: New Event Trigger: table_rewrite

Hi,

Michael Paquier <michael.paquier@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 <simon@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 <robertmhaas@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