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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
