On 2014-10-15 13:15:51 +0900, Michael Paquier wrote:
> On Sat, Oct 11, 2014 at 4:14 AM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> >> When, within a transaction:
> >> * I drop a foreign key (or any) constraint.
> >> * Load some data to the table which won't fit the constraint.
> >> * Analyze the table.
> >> * Attempt to re-add the constraint which fails and should roll back the
> >> whole transaction.
> >>
> >> The constraint is still missing after rollback.
> > That's, like, a *seriously* bad idea. The current xact doesn't see a
> > trigger, so we remove relhastriggers (and similarly other relhas*
> > stuff). The kicker is that we do so *nontransactionally*.
> >
> > That's fine enough for VACUUM because that doesn't run in a
> > transaction. But vac_update_relstats is also run for ANALYZE.
> >
> > I've no time to think this through right now, but my current thinking is
> > that we should use a transactional update for ANALYZE.
>
> The comments on top of vac_update_relstats rely on heap_inplace_update:
> * Note another assumption: that two VACUUMs/ANALYZEs on a
> table can't
> * run in parallel, nor can VACUUM/ANALYZE run in parallel
> with a
> * schema alteration such as adding an index, rule, or
> trigger. Otherwise
> * our updates of relhasindex etc might overwrite uncommitted
> updates.
> I am not sure what would be the side effects of such a change, but it seems
> dangerous to add any control mechanism able to choose if ctup is updated
> with either heap_inplace_update or simple_heap_update, especially something
> like (GetTopTransactionIdIfAny() == InvalidTransactionId) to determine if
> this code path is taken in an xact that has already done a transactional
> update.
I'm not sure which danger you're seeing here. Imo we need to choose
between heap_inplace/heap_update for VACUUM/ANALYZE because one is
allowed to run in a transaction, and the other is not. It simply *can't*
be safe for ANALYZE to set things like relhastriggers = false using
heap_inplace().
There's problems with both it rolling back and thus undoing the action
that allowed relhastriggers = false to be set and scenarios where it's
not ok that other backends can see that value before the transaction
committed.
> Perhaps a solution would be to document properly that analyze
> should not be run in the same transaction as schema changes.
Surely corrupting the database (yes, this is what's happening here),
isn't something that can just be documented away.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services