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

On 6 March 2014 22:43, Noah Misch <noah@leadboat.com> wrote:
> 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.

Agreed

> 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.

Too hacky for me, but some good thinking. My proposed solution is below.

> -- 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().

These are certainly concerning. What surprises me the most is that
pg_dump has been so happy to randomly mix SQL using the transaction
snapshot with sys cache access code using a different snapshot. If
that was intention there is no documentation in code or in the docs to
explain that.

> -- pg_get_viewdef()
>
> Untamed: pg_dump does not lock views at all.

OMG, its really a wonder pg_dump works 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.

The right thing to do here is to not push to the extremes. If we mess
too much with the ruleutil stuff it will just be buggy. A more
considered analysis in a later release is required for a full and
complete approach. As I indicated earlier, an 80/20 solution is better
for this release.

Slimming down the patch, I've removed changes to lock levels for
almost all variants. The only lock levels now reduced are those for
VALIDATE, plus setting of relation and attribute level options.

VALIDATE is implemented by calling pg_get_constraintdef_mvcc(), a
slightly modified variant of pg_get_constraintdef that uses the
transaction snapshot. I propose this rather than Noah's solution
solely because this will allow any user to request the MVCC data,
rather than implement a hack that only works for pg_dump. I will post
the patch later today.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services