Re: ALTER TABLE lock strength reduction patch is unsafe

On Sat, Dec 17, 2011 at 3:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> I am also not entirely sure I believe that this is plugging all the
> failure cases.  I think that it may just be making the failure cases
> more obscure, rather than really getting rid of them.  Consider
> something like the following:
>
> T1: Update row version A, creating new row version B.
> T2: Begin scanning the catalog in question.  We happen to encounter
> row version B first.  We remember T1's XID as in progress, but don't
> see the row since T1 hasn't committed.
> T1: Rollback.
> T3: Update row version A, creating new row version C.
> T3: Commit.
> T2: Scan now comes to row version A; we don't see that version either,
> since T3 is committed.
>
> I don't think there's any guarantee that T2's scan will see tuples
> inserted after the start of the scan.  If I'm correct about that, and
> I'm pretty sure it's true for sequential scans anyway, then T2's scan
> might end without seeing C either.

A simpler example shows it better. Tom's idea prevents 2 rows being
returned, but doesn't prevent zero rows.

That's a useful improvement, but not the only thing.

ISTM we can run a scan as we do now, without locking. If scan returns
zero rows we scan again, but take a definition lock first. ALTER TABLE
holds the definition lock while running, which won't be a problem
because we would only use that on shorter AT statements.

Definition lock being the type of lock described earlier on this
thread, that we already have code for. So we have code for all the
parts we need to make this work.

So that means we'd be able to plug the gaps noted, without reducing
performance on the common code paths.

I don't see any objections made so far remain valid with that approach.

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