Re: ALTER TABLE lock strength reduction patch is unsafe Reply-To: - Mailing list pgsql-hackers

On Tue, Mar 04, 2014 at 06:50:17PM +0100, Andres Freund wrote:
> On 2014-03-04 11:40:10 -0500, Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes: > I think this is all too
> > late for 9.4, though.
> > 
> > I agree with the feeling that a meaningful fix for pg_dump isn't going
> > to get done for 9.4.  So that leaves us with the alternatives of (1)
> > put off the lock-strength-reduction patch for another year; (2) push
> > it anyway and accept a reduction in pg_dump reliability.
> > 
> > I don't care for (2).  I'd like to have lock strength reduction as
> > much as anybody, but it can't come at the price of reduction of
> > reliability.
> 
> I am sorry, but I think this is vastly overstating the scope of the
> pg_dump problem. CREATE INDEX *already* doesn't require a AEL, and the
> amount of problems that has caused in the past is surprisingly low. If
> such a frequently used command doesn't cause problems, why are you
> assuming other commands to be that problematic? And I think it's hard to
> argue that the proposed changes are more likely to cause problems.
> 
> Let's try to go at this a bit more methodically. The commands that -
> afaics - change their locklevel due to latest patch (v21) are:
[snip]

Good analysis.  The hazards arise when pg_dump uses one of the ruleutils.c
deparse worker functions.  As a cross-check to your study, I looked briefly at
the use of those functions in pg_dump and how this patch might affect them:

-- pg_get_constraintdef()

pg_dump reads the constraint OID with its transaction snapshot, so we will
never see a too-new constraint.  Dropping a constraint still requires
AccessExclusiveLock.

Concerning VALIDATE CONSTRAINT, pg_dump reads convalidated with its
transaction snapshot and uses that to decide whether to dump the CHECK
constraint as part of the CREATE TABLE or as a separate ALTER TABLE ADD
CONSTRAINT following the data load.  However, pg_get_constraintdef() reads the
latest convalidated to decide whether to emit NOT VALID.  Consequently, one
can get a dump in which the dumped table data did not yet conform to the
constraint, and the ALTER TABLE ADD CONSTRAINT (w/o NOT VALID) fails.
(Suppose you deleted the last invalid rows just before executing the VALIDATE
CONSTRAINT.  I tested this by committing the DELETE + VALIDATE CONSTRAINT with
pg_dump stopped at getTableAttrs().)

One hacky, but maintainable and effective, solution to the VALIDATE CONSTRAINT
problem is to have pg_dump tack on a NOT VALID if pg_get_constraintdef() did
not do so.  It's, conveniently, the last part of the definition.  I would tend
to choose this.  We could also just decide this isn't bad enough to worry
about.  The consequence is that an ALTER TABLE ADD CONSTRAINT fails.  Assuming
no --single-transaction for the original restoral, you just add NOT VALID to
the command and rerun.  Like most of the potential new pg_dump problems, this
can already happen today if the relevant database changes happen between
taking the pg_dump transaction snapshot and locking the tables.

-- pg_get_expr() for default expressions

pg_dump reads pg_attrdef.adbin using its transaction snapshot, so it will
never see a too-new default.  This does allow us to read a dropped default.
That's not a problem directly.  However, suppose the default references a
function dropped at the same time as the default.  pg_dump could fail in
pg_get_expr().

-- pg_get_indexdef()

As you explained elsewhere, new indexes are no problem.  DROP INDEX still
requires AccessExclusiveLock.  Overall, no problems here.

-- pg_get_ruledef()

The patch changes lock requirements for enabling and disabling of rules, but
that is all separate from the rule expression handling.  No problems.

-- pg_get_triggerdef()

The patch reduces CREATE TRIGGER and DROP TRIGGER to ShareUpdateExclusiveLock.
The implications for pg_dump are similar to those for pg_get_expr().

-- pg_get_viewdef()

Untamed: pg_dump does not lock views at all.


One thing not to forget is that you can always get the old mutual exclusion
back by issuing LOCK TABLE just before a DDL operation.  If some unlucky user
regularly gets pg_dump failures due to concurrent DROP TRIGGER, he has a
workaround.  There's no comparable way for someone who would not experience
that problem to weaken the now-hardcoded AccessExclusiveLock.  Many
consequences of insufficient locking are too severe for that workaround to
bring comfort, but the pg_dump failure scenarios around pg_get_expr() and
pg_get_triggerdef() seem mild enough.  Restore-time failures are more serious,
hence my recommendation to put a hack in pg_dump around VALIDATE CONSTRAINT.

Thanks,
nm

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com