Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

Michael Paquier <michael.paquier@gmail.com> writes:
> On Wed, Oct 15, 2014 at 3:18 PM, Andres Freund <andres@2ndquadrant.com>
>> On 2014-10-15 15:02:43 +0900, Michael Paquier wrote:
>>> Btw, I have just put my hands on this code and made the attached to
>>> make vac_update_relstats able to do a transactional update. It looks
>>> to work fine with only a check on the flags of vacuum statement.

I think the original reasoning why this would be okay even for ANALYZE
was that if we are doing an ANALYZE inside a transaction that has done
DDL on the table, it would be okay to modify the pg_class row in-place
because it would be a tuple that the transaction itself has written
and so it would be invalidated if the transaction rolled back.  The flaw
in this reasoning is that the DDL might not actually have changed the
pg_class row, and so it might still be the original pre-transaction
version that's visible to other transactions.  Thus the risk is confined
to cases like DROP INDEX that don't modify pg_class.  I'm not entirely
sure, but this reasoning might have been perfectly correct at the time
and have been invalidated by later optimizations to suppress "unnecessary"
physical updates of pg_class in favor of just sending sinval events.
(Note that DROP INDEX certainly must send a relcache inval to ensure that
other sessions stop using the index, and at one time the only trigger for
such an inval was to physically update pg_class and/or pg_attribute.)

>> Imo this is complex enough to deserve a regression test. Can you add
>> one?

> Definitely makes sense. Here is an updated version.

I don't much care for this patch.  Aside from cosmetic issues like having
named the new argument backwards and failed to update the function header
comment that the patch largely invalidates, it seems to me to be likely
to have unforeseen side effects, in that there may now be assumptions
elsewhere that we don't force a pg_class update for this type of change.
The implications are particularly ticklish for pg_class itself...

I think that a better answer is to continue to do this update
nontransactionally, but to not let the code clear relhasindex etc
if we're inside a transaction block.  It is certainly safe to put
off clearing those flags if we're not sure that we're seeing a
committed state of the table's schema.

An interesting question is whether it is ever possible for this function
to be told to *set* relhasindex when it was clear (or likewise for the
other flags).  Offhand I would say that that should never happen, because
certainly neither VACUUM nor ANALYZE should be creating indexes etc.
Should we make it throw an error if that happens, or just go ahead and
apply the update, assuming that it's correcting somehow-corrupted data?

            regards, tom lane