Re: ALTER TABLE lock strength reduction patch is unsafe Reply-To:

On 21 March 2014 03:45, Noah Misch <noah(at)leadboat(dot)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