Re: New Event Trigger: table_rewrite

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Simon Riggs <simon(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: New Event Trigger: table_rewrite
Date: 2014-11-18 22:14:55
Message-ID: m2389gp34w.fsf@2ndQuadrant.fr
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

Michael Paquier <michael(dot)paquier(at)gmail(dot)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(at)2ndquadrant(dot)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(at)gmail(dot)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

Attachment Content-Type Size
table_rewrite.3.patch text/x-patch 60.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2014-11-18 22:16:01 Re: Additional role attributes && superuser review
Previous Message Petr Jelinek 2014-11-18 22:05:14 Re: Add shutdown_at_recovery_target option to recovery.conf