Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1n8lTR-0006PC-Kh for pgsql-bugs@arkaria.postgresql.org; Sat, 15 Jan 2022 16:00:13 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1n8lTQ-0002En-4B for pgsql-bugs@arkaria.postgresql.org; Sat, 15 Jan 2022 16:00:12 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1n8lTP-0002EZ-Nu for pgsql-bugs@lists.postgresql.org; Sat, 15 Jan 2022 16:00:11 +0000 Received: from mail-lf1-x135.google.com ([2a00:1450:4864:20::135]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1n8lTI-0004Hd-EI for pgsql-bugs@lists.postgresql.org; Sat, 15 Jan 2022 16:00:11 +0000 Received: by mail-lf1-x135.google.com with SMTP id b14so22262581lff.3 for ; Sat, 15 Jan 2022 08:00:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language; bh=lvrHjBF/FXGeB/EJafKQ4hRcn9oXepbPdr3GWj/Hxb8=; b=AHl/RQ8Hvmh5q3n5FVVXRwbvZ7JM9W0yvkDoXjh8D/ziWok1Am0/dqpPFq33oDU1Xf VFgGOk1LibyAwswft7NS+3nWOMCA+JXFV6gR2VflFU8aHXt+iYxO23jgVJdkrSWE2vuC O87H3zK525PJ+C+SdY0GvjmpEj8M+76WM8NKTRubfQO+1wGMVtLFy81xJag4HlvJU2Xf T2n3TMhSFxt1KEhsWVbjJSV8PacuqmZyB68gakeTmB9AnaAb3Lm4knhKFzY4JqelaOIr ++iRh876iCWkJg1GKHyqp+IFfzfcMM5f+FvbsCMvXgQpxGfL0ncNGdBzIS6sF09S3Gh6 MR4g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=lvrHjBF/FXGeB/EJafKQ4hRcn9oXepbPdr3GWj/Hxb8=; b=WraJf67XaPAHwSfX62wPzIS5wBq1AAhEtjdztpegICrg2+Owrd4SuclyshLrScmfbI EFWiNMgkqxmLkeFdSse5ueDeKn9f/ehym3zWYhn8pQn0oq15XgYkDTuMOX/43aGgSzs/ J89MXolhBVa8pBRMWUPl3dfIvyLoY8Yj7zPFK7tSrXakQwKvjlzXFBmCTliquVsQd9/a XD9xm37oK40Rcdu1LAr1ZYqzRMp6gn3EmbXPuqzpkk8UO5zFIH9BOIcWu81cyI9b3IR/ yK2W26z1EU5yLA7UeeULeWF0YUWwi9WWjZdRS6Lo1iF5U83AGckX9/4zJXLFrQcnzk1A uYhQ== X-Gm-Message-State: AOAM530kWvLqwdcYUTYxkeCRtW8lqIf9I8NkHAa/d1ALB1FejUEe+Pzy ihFuRqOI7vwU1+J6oz+WIo4bX4KJlQ8= X-Google-Smtp-Source: ABdhPJyYTiRYMlgFhNyj6uPDN87CC8aX++ZFWyzPqYRhjDLgTUpHvaTSnm3LGQ86p3kXCBR8W3ZuIw== X-Received: by 2002:a2e:7216:: with SMTP id n22mr9915561ljc.170.1642262403309; Sat, 15 Jan 2022 08:00:03 -0800 (PST) Received: from [1.0.0.7] ([178.155.6.39]) by smtp.gmail.com with ESMTPSA id b18sm840836ljq.62.2022.01.15.08.00.01 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 15 Jan 2022 08:00:01 -0800 (PST) Subject: Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction To: pgsql-bugs@lists.postgresql.org References: <17368-98a4f99e8e4b4402@postgresql.org> From: Alexander Lakhin Message-ID: <18a116d2-7927-6a62-aa60-ab16c53183f9@gmail.com> Date: Sat, 15 Jan 2022 19:00:00 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <17368-98a4f99e8e4b4402@postgresql.org> Content-Type: multipart/mixed; boundary="------------816835A0DE812272F88FFFE8" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------816835A0DE812272F88FFFE8 Content-Type: multipart/alternative; boundary="------------F74C8523842AFB2144F83752" --------------F74C8523842AFB2144F83752 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 15.01.2022 11:00, PG Bug reporting form wrote: > The following bug has been logged on the website: > > Bug reference: 17368 This bug is separated from the #17116, because it's an unrelated issue. But it was discussed in the thread #17116 and there was proposed the working v4-0002-Fix-race-in-SERIALIZABLE-READ-ONLY.patch [1]. I've attached the modification for the read-only-anomaly-3 test to reproduce the bug (the result is shown in the bug report). For ease of reading, all relevant info quoted here: > On Wed, Jul 28, 2021 at 5:02 PM Thomas Munro wrote: >> On Wed, Jul 28, 2021 at 9:00 AM Alexander Lakhin wrote: >>> (Except for read-only-anomaly-3, that causes another >>> assertion fail but I believe that it's not related to the initial issue >>> and deserves another bug report.) >> Huh. I tried and failed to find that one with concurrent runs. I'll >> wait for your next report before I do anything, just in case there's a >> connection. > This is indeed an unrelated issue, and much older. After many > kilowatt hours, I reproduced it here and worked out that it comes from > GetSafeSnapshot() not expecting possibleUnsafeConflicts to be empty > when WritableSxactCount == 0. More soon. > On Fri, Jul 30, 2021 at 9:41 AM Thomas Munro wrote: > The problem is that if we don't take the fast exit here: > > if (XactReadOnly && PredXact->WritableSxactCount == 0) > { > ReleasePredXact(sxact); > LWLockRelease(SerializableXactHashLock); > return snapshot; > } > > ... then we search for writable SSI transactions here: > > for (othersxact = FirstPredXact(); > othersxact != NULL; > othersxact = NextPredXact(othersxact)) > { > if (!SxactIsCommitted(othersxact) > && !SxactIsDoomed(othersxact) > && !SxactIsReadOnly(othersxact)) > { > SetPossibleUnsafeConflict(sxact, othersxact); > } > } > > ... but that can find nothing if all writers happen to be doomed. > WritableSxactCount doesn't count read-only or committed transactions > so we don't have to worry about those confusing us, but it does count > doomed transactions. Having an empty list here is mostly fine, except > that nobody will ever set our RO_SAFE flag, and in the case of > DEFERRABLE, the assertion that someone has set it will fail. This is > the case since: > > === > commit bdaabb9b22caa71021754d3967b4032b194d9880 > Author: Heikki Linnakangas > Date: Fri Jul 8 00:36:30 2011 +0300 > > There's a small window wherein a transaction is committed but not yet > on the finished list, and we shouldn't flag it as a potential conflict > if so. We can also skip adding a doomed transaction to the list of > possible conflicts because we know it won't commit. > > Dan Ports and Kevin Grittner. > === > > I see three fixes: > > 1. Check if the list is empty, and if so, set our own > SXACT_FLAG_RO_SAFE. Then the assertion in GetSafeSnapshot() will > pass, and also non-DEFERRABLE callers will eventually see the flag and > opt out. > > 2. Check if the list is empty, and if so, opt out immediately (as we > do when WriteableSxactCount == 0). This would be strictly better than > #1, because it happens sooner while we still have the lock. On the > other hand, we'd have to undo more effects, and that involves writing > more fragile and very rarely run code. > > 3. Just delete the assertion in GetSafeSnapshot(). This is > attractively simple but it means that READ ONLY non-DEFERRABLE > transactions arbitrarily miss out on the RO_SAFE optimisation in this > (admittedly rare) case. > > The attached 0002 has idea #1, which I prefer for back-patching. I > think #2 might be a good idea if we take the filtering logic further > so that it becomes more likely that you get an empty list (for > example, why don't we skip transactions that are running in a > different database?), but then that'd be new code, not something we > back-patch. Thoughts? [1] https://www.postgresql.org/message-id/CA%2BhUKGJd-%3DmdUdS%2BGh-FN4rZgAg4M-V%3D%3DG7FMCU-KbUffyPJDw%40mail.gmail.com Best regards, Alexander --------------F74C8523842AFB2144F83752 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit
15.01.2022 11:00, PG Bug reporting form wrote:
The following bug has been logged on the website:

Bug reference:      17368
This bug is separated from the #17116, because it's an unrelated issue. But it was discussed in the thread #17116 and there was proposed the working v4-0002-Fix-race-in-SERIALIZABLE-READ-ONLY.patch [1].
I've attached the modification for the read-only-anomaly-3 test to reproduce the bug (the result is shown in the bug report).
For ease of reading, all relevant info quoted here:
On Wed, Jul 28, 2021 at 5:02 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Wed, Jul 28, 2021 at 9:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:

(Except for read-only-anomaly-3, that causes another
assertion fail but I believe that it's not related to the initial issue
and deserves another bug report.)

Huh.  I tried and failed to find that one with concurrent runs.  I'll
wait for your next report before I do anything, just in case there's a
connection.

This is indeed an unrelated issue, and much older.  After many
kilowatt hours, I reproduced it here and worked out that it comes from
GetSafeSnapshot() not expecting possibleUnsafeConflicts to be empty
when WritableSxactCount == 0.  More soon.


On Fri, Jul 30, 2021 at 9:41 AM Thomas Munro <thomas.munro@gmail.com> wrote:

The problem is that if we don't take the fast exit here:

    if (XactReadOnly && PredXact->WritableSxactCount == 0)
    {
        ReleasePredXact(sxact);
        LWLockRelease(SerializableXactHashLock);
        return snapshot;
    }

... then we search for writable SSI transactions here:

        for (othersxact = FirstPredXact();
             othersxact != NULL;
             othersxact = NextPredXact(othersxact))
        {
            if (!SxactIsCommitted(othersxact)
                && !SxactIsDoomed(othersxact)
                && !SxactIsReadOnly(othersxact))
            {
                SetPossibleUnsafeConflict(sxact, othersxact);
            }
        }

... but that can find nothing if all writers happen to be doomed.
WritableSxactCount doesn't count read-only or committed transactions
so we don't have to worry about those confusing us, but it does count
doomed transactions.  Having an empty list here is mostly fine, except
that nobody will ever set our RO_SAFE flag, and in the case of
DEFERRABLE, the assertion that someone has set it will fail.  This is
the case since:

===
commit bdaabb9b22caa71021754d3967b4032b194d9880
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date:   Fri Jul 8 00:36:30 2011 +0300

    There's a small window wherein a transaction is committed but not yet
    on the finished list, and we shouldn't flag it as a potential conflict
    if so. We can also skip adding a doomed transaction to the list of
    possible conflicts because we know it won't commit.

    Dan Ports and Kevin Grittner.
===

I see three fixes:

1.  Check if the list is empty, and if so, set our own
SXACT_FLAG_RO_SAFE.  Then the assertion in GetSafeSnapshot() will
pass, and also non-DEFERRABLE callers will eventually see the flag and
opt out.

2.  Check if the list is empty, and if so, opt out immediately (as we
do when WriteableSxactCount == 0).  This would be strictly better than
#1, because it happens sooner while we still have the lock.  On the
other hand, we'd have to undo more effects, and that involves writing
more fragile and very rarely run code.

3.  Just delete the assertion in GetSafeSnapshot().  This is
attractively simple but it means that READ ONLY non-DEFERRABLE
transactions arbitrarily miss out on the RO_SAFE optimisation in this
(admittedly rare) case.

The attached 0002 has idea #1, which I prefer for back-patching.  I
think #2 might be a good idea if we take the filtering logic further
so that it becomes more likely that you get an empty list (for
example, why don't we skip transactions that are running in a
different database?), but then that'd be new code, not something we
back-patch.  Thoughts?
[1] https://www.postgresql.org/message-id/CA%2BhUKGJd-%3DmdUdS%2BGh-FN4rZgAg4M-V%3D%3DG7FMCU-KbUffyPJDw%40mail.gmail.com

Best regards,
Alexander
--------------F74C8523842AFB2144F83752-- --------------816835A0DE812272F88FFFE8 Content-Type: text/x-patch; charset=UTF-8; name="read-only-anomaly-3+.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="read-only-anomaly-3+.patch" diff --git a/src/test/isolation/specs/read-only-anomaly-3.spec b/src/test/isolation/specs/read-only-anomaly-3.spec index 61d9c0b9e0f..80a38b5d4aa 100644 --- a/src/test/isolation/specs/read-only-anomaly-3.spec +++ b/src/test/isolation/specs/read-only-anomaly-3.spec @@ -36,4 +36,11 @@ setup { BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; } step s3r { SELECT id, balance FROM bank_account WHERE id IN ('X', 'Y') ORDER BY id; } step s3c { COMMIT; } +session s4 +setup { BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; } +step s4to { SET lock_timeout = '10ms' } +step s4wx { UPDATE bank_account SET balance = -11 WHERE id = 'X'; } +step s4c { COMMIT; } + permutation s2rx s2ry s1ry s1wy s1c s3r s2wx s2c s3c +permutation s2ry s1wy s1c s2wx s4to s4wx s4c s3r s2c s3c --------------816835A0DE812272F88FFFE8--