Re: ALTER TABLE lock strength reduction patch is unsafe

On Mon, Mar 03, 2014 at 07:19:45PM +0000, Simon Riggs wrote:
> On 3 March 2014 18:57, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Mon, Mar 03, 2014 at 10:19:55AM -0500, Robert Haas wrote:
> >> On Thu, Feb 27, 2014 at 3:12 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> > Removing SELECT privilege while running a SELECT would be a different
> >> > matter. This is all a matter of definition; we can make up any rules
> >> > we like. Doing so is IMHO a separate patch and not something to hold
> >> > up the main patch.
> >>
> >> So I think this is an interesting point. There are various things
> >> that could go wrong as a result of using the wrong lock level. Worst
> >> would be that the server crashes or corrupts data. A little less bad
> >> would be that sessions error out with inexplicable error conditions,
> >> as in SnapshotNow days. Alternatively, we could just have arguably
> >> wrong behavior, like basing query results on the old version of the
> >> table's metadata even after it's been changed.
> >
> > I would order the concerns like this:
> >
> > 1. Data corruption
> > 2. Transient, clearly-wrong answers without an error
> > 3. Server crash
> > 4. Catalog logical inconsistency
> > 5. Inexplicable, transient errors
> > 6. Valid behavior capable of surprising more than zero upgraders
>
> I like your model for risk assessment. How can we apply it in detail
> in a way that helps us decide? Or do we just go on gut feel?
>
> My experience with mentioning such topics is that without structure it
> results in an assessment of "unacceptable risk" just simply because
> somebody has mentioned some scary words.

True; it's tough to make use of such a prioritization. By the time you can
confidently assign something to a category, you can probably just fix it.
(Or, in the case of category (6), document/release-note it.)

Just to be clear, that list is not a commentary on the particular patch at
hand. Those are merely the kinds of regressions to look for in a patch
affecting this area of the code.

> err, guys, you do realise that changing ownership is staying at
> AccessExclusiveLock in this patch?
> (and I haven't ever suggested lowering that).

Yep.

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