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

On 21 March 2014 03:45, Noah Misch <noah@leadboat.com> wrote:

>> + * Note that Hot Standby only knows about AccessExclusiveLocks on the master
>> + * so any changes that might affect SELECTs running on standbys need to use
>> + * AccessExclusiveLocks even if you think a lesser lock would do, unless you
>> + * have a solution for that also.
>
> Out of curiosity, do SELECTs on hot standbys impose known challenges in this
> area not shared with local SELECTs?

No, but locks less than AccessExclusiveLock won't happen at all, so
its a difference that if improperly handled could cause a bug.

Plus I wanted to indicate I'd thought about it.

>> -      * 2. Relcache needs to be internally consistent, so unless we lock the
>> -      * definition during reads we have no way to guarantee that.
>
> I looked for hazards like this, but I found none in the ALTER forms covered by
> this patch.  None of them modify multiple catalog rows affecting the same
> relcache entry.  However, thinking about that did lead me to ponder another
> class of hazards.  When backends can use one or more relations concurrently
> with a DDL operation affecting those relations, those backends can find
> themselves running with a subset of the catalog changes made within a
> particular DDL operation.  Consider VALIDATE CONSTRAINT against an inherited
> constraint of an inheritance parent.  It validates child table constraints,
> modifying one catalog row per table.  At COMMIT time, we queue sinval messages
> for all affected tables.  We add to the queue in atomic groups of
> WRITE_QUANTUM (64) messages.  Between two such groups joining the queue,
> another backend may process the first group of messages.  If the original DDL
> used AccessExclusiveLock, this is always harmless.  The DDL-issuing backend
> still holds its lock, which means the inval-accepting backend must not have
> the relation open.  If the inval-accepting backend later opens the affected
> relation, it will first acquire some lock and process the rest of the
> invalidations from the DDL operation.  When doing DDL under a weaker lock, the
> inval-accepting backend might apply half the invalidations and immediately use
> them in the context of an open relation.  For VALIDATE CONSTRAINT, this means
> a backend might briefly recognize only a subset of the inheritance tree
> becoming valid.  (I did not actually build a test case to confirm this.)
>
> Considering that constraint exclusion is the sole consumer of
> convalidated/ccvalid that can run in parallel with VALIDATE CONSTRAINT, I
> think this is harmless.  I did not find problems of this nature in any ALTER
> TABLE forms affected by the patch.  Let's just keep it in mind during future
> lock level changes.

I'll document

> pg_get_constraintdef_mvcc() still does syscache lookups by way of
> decompile_column_index_array(), get_constraint_index(), and
> deparse_expression_pretty().  It uses MVCC for things that matter for pg_dump
> vs. reduced lock levels, but not comprehensively.  I recommend not adding a
> new function and instead changing pg_get_constraintdef() to use the
> transaction snapshot unconditionally.

OK

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