Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> 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.
Actually when you do a CLUSTER or a VACUUM FULL you know that the
table is going to be rewritten on disk, because that's about the only
purpose of the command.
Given the complexity involved here, the new version of the patch
(attached) has removed support for those statements.
> 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?
Fixed in the attached version of the patch.
> 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.
Fixed in the attached version of the patch.
> You still have the ANALYZE thing in docs, which now should be removed.
Fixed in the attached version of the patch.
--
Dimitri Fontaine 06 63 07 10 78
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
