pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
12+ messages / 4 participants
[nested] [flat]
* BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
@ 2022-01-15 08:00 PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: PG Bug reporting form @ 2022-01-15 08:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 17368
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 14.1
Operating system: Ubuntu 20.04
Description:
The isolation test "read-only-anomaly-3" modified as follows:
setup
{
CREATE TABLE bank_account (id TEXT PRIMARY KEY, balance DECIMAL NOT
NULL);
INSERT INTO bank_account (id, balance) VALUES ('X', 0), ('Y', 0);
}
teardown
{
DROP TABLE bank_account;
}
session s1
setup { BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; }
step s1ry { SELECT balance FROM bank_account WHERE id = 'Y'; }
step s1wy { UPDATE bank_account SET balance = 20 WHERE id = 'Y'; }
step s1c { COMMIT; }
session s2
setup { BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; }
step s2rx { SELECT balance FROM bank_account WHERE id = 'X'; }
step s2ry { SELECT balance FROM bank_account WHERE id = 'Y'; }
step s2wx { UPDATE bank_account SET balance = -11 WHERE id = 'X'; }
step s2c { COMMIT; }
session s3
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
causes an assertion failure with the following stacktrace:
Core was generated by `postgres: law isolation_regression [local] SELECT
'.
Program terminated with signal SIGABRT, Aborted.
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007f53a8a7c859 in __GI_abort () at abort.c:79
#2 0x000055edb1e89381 in ExceptionalCondition (conditionName=0x55edb2832980
"SxactIsROSafe(MySerializableXact)",
errorType=0x55edb2831da0 "FailedAssertion", fileName=0x55edb2831d60
"predicate.c", lineNumber=1612) at assert.c:69
#3 0x000055edb1714442 in GetSafeSnapshot (origSnapshot=0x55edb6ac6e40
<CurrentSnapshotData>) at predicate.c:1612
#4 0x000055edb1714b91 in GetSerializableTransactionSnapshot
(snapshot=0x55edb6ac6e40 <CurrentSnapshotData>)
at predicate.c:1703
#5 0x000055edb201d623 in GetTransactionSnapshot () at snapmgr.c:291
#6 0x000055edb17613b5 in exec_simple_query (
query_string=0x625000005220 "SELECT id, balance FROM bank_account WHERE
id IN ('X', 'Y') ORDER BY id;")
at postgres.c:1103
#7 0x000055edb176ea2e in PostgresMain (argc=1, argv=0x7ffdfa7290d0,
dbname=<optimized out>,
username=0x629000011258 "law") at postgres.c:4486
#8 0x000055edb13ea951 in BackendRun (port=0x615000004900) at
postmaster.c:4530
#9 0x000055edb13e9142 in BackendStartup (port=0x615000004900) at
postmaster.c:4252
#10 0x000055edb13df0a3 in ServerLoop () at postmaster.c:1745
#11 0x000055edb13dcf20 in PostmasterMain (argc=8, argv=0x607000000170) at
postmaster.c:1417
#12 0x000055edb0f075c3 in main (argc=8, argv=0x607000000170) at main.c:209
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
@ 2022-01-15 16:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Alexander Lakhin @ 2022-01-15 16:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
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%...
Best regards,
Alexander
Attachments:
read-only-anomaly-3+.patch (text/x-patch, 792B) download
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
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
@ 2023-03-06 03:02 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Munro @ 2023-03-06 03:02 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Sun, Jan 16, 2022 at 5:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>> 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?
While swapping unrelated bug #17116 back into my brain, I remembered
this one. Coincidentally, I also had a note on my to-do list to
investigate a problem that has started showing up on CI, in the new
"running" tests (which seem to be finding fun new timing bugs in
committed code):
http://cfbot.cputube.org/highlights/assertion-30.html
Duh, it's this same ancient bug that we already figured out, which
dates from 2011. I plan to do some more testing and then proceed with
the idea mentioned above[1].
[1] https://www.postgresql.org/message-id/attachment/125053/v4-0002-Fix-race-in-SERIALIZABLE-READ-ONLY.p...
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
@ 2023-03-06 23:39 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:42 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Tom Lane <tgl@sss.pgh.pa.us>
2023-03-08 11:57 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
0 siblings, 2 replies; 12+ messages in thread
From: Thomas Munro @ 2023-03-06 23:39 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Mon, Mar 6, 2023 at 4:02 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> Duh, it's this same ancient bug that we already figured out, which
> dates from 2011. I plan to do some more testing and then proceed with
> the idea mentioned above[1].
Scratch that. I like the more ambitious idea better. I originally
thought it would be less back-patchable, but that's nonsense -- this
was a straight up thinko in ancient commit bdaabb9b and we should just
fix it properly. That is, we should treat this case as a
generalisation of the the PredXact->WritableSxactCount == 0 case, it's
just that we had to do more work to figure that out. Here's a patch
like that.
As mentioned, this approach allows for more improvements in later
patches. There is absolutely no reason for transactions in separate
databases to be in each others' possibleUnsafeConflicts lists. At
least, as long as PostgreSQL maintains its strict
no-cross-database-access policy, which I don't see changing any time
soon. So here is a draft patch like that, for the next commitfest.
It's a little easier to exercise than the "everything-is-doomed" case.
When I have more time I'll try to make proper tests, but basically you
just need two databases:
s1: \c db1
s1: CREATE TABLE t (i int);
s1: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
s1: INSERT INTO t VALUES (42);
s2: \c db2
s2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE;
s2: SELECT * FROM x;
s2 shouldn't have to wait, and doesn't with this patch. But it does
if it's in db1 instead.
Attachments: download all (2 files, .tar.gz)
v3-0001-Fix-race-in-SERIALIZABLE-READ-ONLY.patch (text/x-patch, 2.0K) download
From 496966a89acca5eb8e406571637ad00d0135cb5c Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Fri, 30 Jul 2021 17:27:16 +1200
Subject: [PATCH v3 1/2] Fix race in SERIALIZABLE READ ONLY.
Commit bdaabb9b started skipping doomed transactions when building the
list of possible conflicts, when obtaining a snapshot for SERIALIZABLE
READ ONLY. This had two unintended consequences:
1. With unlucky timing, a transaction will never benefit from the
SXACT_FLAG_RO_SAFE optimization, because no one will ever set the flag.
2. In the same circumstances but with DEFERRABLE, an assertion that
SXACT_FLAG_RO_SAFE has been set fails.
This is really exactly the same as the "opt out" case for
PredXact->WritableSxactCount == 0, it just happens a bit later in racy
conditions. We should "opt out" in the same way in this case too.
Back-patch to all supported releases. Discovered along with bug #17116,
but a separate commit to back-patch further.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/17116-d6ca217acc180e30%40postgresql.org
---
src/backend/storage/lmgr/predicate.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 6b5a416873..99386d36a2 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1839,6 +1839,19 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
SetPossibleUnsafeConflict(sxact, othersxact);
}
}
+
+ /*
+ * If we didn't find any possibly unsafe conflicts because every
+ * uncommitted writable transaction turned out to be doomed (which
+ * should be rare), then we can "opt out" immediately. This is a
+ * variation of the opt out for PredXact->WritableSxactCount == 0.
+ */
+ if (dlist_is_empty(&sxact->possibleUnsafeConflicts))
+ {
+ ReleasePredXact(sxact);
+ LWLockRelease(SerializableXactHashLock);
+ return snapshot;
+ }
}
else
{
--
2.39.1
v3-0002-Optimize-cross-database-SERIALIZABLE-safe-snapsho.patch (text/x-patch, 5.3K) download
From f7f64db598733f5258e9d4be304aadd62e54af30 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 7 Mar 2023 11:36:44 +1300
Subject: [PATCH v3 2/2] Optimize cross-database SERIALIZABLE safe snapshots.
SERIALIZABLE READ ONLY [DEFERRABLE] transactions can benefit from "safe
snapshots", where they detect that no serializable anomalies could be
created, so we can silently drop to the cheaper REPEATABLE READ (in
other words from SSI to SI).
Since a transactions connected to different databases can't access each
others' data at all, except for catalogs where SSI doesn't apply anyway,
there is no point in waiting for transactions in other databases.
Filter them out while populating our possibleUnsafeConflicts lists.
This means that non-DEFERRABLE safe snapshots might opt out immediately
or sooner, and DEFERRABLE safe snapshots might not have to wait as long
to get started, in scenarios where more than one database is using
SERIALIZABLE transactions.
Discussion: https://postgr.es/m/17116-d6ca217acc180e30%40postgresql.org
---
src/backend/storage/lmgr/predicate.c | 28 +++++++++++++++++------
src/include/storage/predicate_internals.h | 1 +
2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 99386d36a2..c4b983ea55 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1213,6 +1213,7 @@ InitPredicateLocks(void)
PredXact->OldCommittedSxact->flags = SXACT_FLAG_COMMITTED;
PredXact->OldCommittedSxact->pid = 0;
PredXact->OldCommittedSxact->pgprocno = INVALID_PGPROCNO;
+ PredXact->OldCommittedSxact->database = InvalidOid;
}
/* This never changes, so let's keep a local copy. */
OldCommittedSxact = PredXact->OldCommittedSxact;
@@ -1571,7 +1572,7 @@ GetSafeSnapshotBlockingPids(int blocked_pid, int *output, int output_size)
dlist_foreach(iter, &PredXact->activeList)
{
SERIALIZABLEXACT *sxact =
- dlist_container(SERIALIZABLEXACT, xactLink, iter.cur);
+ dlist_container(SERIALIZABLEXACT, xactLink, iter.cur);
if (sxact->pid == blocked_pid)
{
@@ -1813,6 +1814,7 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
sxact->xmin = snapshot->xmin;
sxact->pid = MyProcPid;
sxact->pgprocno = MyProc->pgprocno;
+ sxact->database = MyDatabaseId;
dlist_init(&sxact->predicateLocks);
dlist_node_init(&sxact->finishedLink);
sxact->flags = 0;
@@ -1832,7 +1834,17 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
{
othersxact = dlist_container(SERIALIZABLEXACT, xactLink, iter.cur);
- if (!SxactIsCommitted(othersxact)
+ /*
+ * We can't possibly have an unsafe conflict with a transaction in
+ * another database. The only possible overlap is on shared
+ * catalogs, but we don't support SSI for shared catalogs. The
+ * invalid database case covers 2PC, because we don't yet record
+ * database OIDs in the 2PC information. We also filter out doomed
+ * transactions as they can't possibly commit.
+ */
+ if ((othersxact->database == InvalidOid ||
+ othersxact->database == MyDatabaseId)
+ && !SxactIsCommitted(othersxact)
&& !SxactIsDoomed(othersxact)
&& !SxactIsReadOnly(othersxact))
{
@@ -1842,9 +1854,10 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
/*
* If we didn't find any possibly unsafe conflicts because every
- * uncommitted writable transaction turned out to be doomed (which
- * should be rare), then we can "opt out" immediately. This is a
- * variation of the opt out for PredXact->WritableSxactCount == 0.
+ * uncommitted writable transaction turned out to be in another
+ * database or doomed (which should be rare), then we can "opt out"
+ * immediately. This is a variation of the opt out for
+ * PredXact->WritableSxactCount == 0.
*/
if (dlist_is_empty(&sxact->possibleUnsafeConflicts))
{
@@ -3564,8 +3577,8 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
* xmin and purge any transactions which finished before this transaction
* was launched.
*
- * For parallel queries in read-only transactions, it might run twice.
- * We only release the reference on the first call.
+ * For parallel queries in read-only transactions, it might run twice. We
+ * only release the reference on the first call.
*/
needToClear = false;
if ((partiallyReleasing ||
@@ -4875,6 +4888,7 @@ predicatelock_twophase_recover(TransactionId xid, uint16 info,
sxact->vxid.localTransactionId = (LocalTransactionId) xid;
sxact->pid = 0;
sxact->pgprocno = INVALID_PGPROCNO;
+ sxact->database = InvalidOid;
/* a prepared xact hasn't committed yet */
sxact->prepareSeqNo = RecoverySerCommitSeqNo;
diff --git a/src/include/storage/predicate_internals.h b/src/include/storage/predicate_internals.h
index 142a195d0e..7ce2882196 100644
--- a/src/include/storage/predicate_internals.h
+++ b/src/include/storage/predicate_internals.h
@@ -116,6 +116,7 @@ typedef struct SERIALIZABLEXACT
uint32 flags; /* OR'd combination of values defined below */
int pid; /* pid of associated process */
int pgprocno; /* pgprocno of associated process */
+ Oid database; /* which database is this transaction in? */
} SERIALIZABLEXACT;
#define SXACT_FLAG_COMMITTED 0x00000001 /* already committed */
--
2.39.1
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
@ 2023-03-06 23:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-07 01:14 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 12+ messages in thread
From: Tom Lane @ 2023-03-06 23:42 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Thomas Munro <thomas.munro@gmail.com> writes:
> As mentioned, this approach allows for more improvements in later
> patches. There is absolutely no reason for transactions in separate
> databases to be in each others' possibleUnsafeConflicts lists.
What if they both touched/modified a shared catalog?
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:42 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-03-07 01:14 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-07 01:16 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Munro @ 2023-03-07 01:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Tue, Mar 7, 2023 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > As mentioned, this approach allows for more improvements in later
> > patches. There is absolutely no reason for transactions in separate
> > databases to be in each others' possibleUnsafeConflicts lists.
>
> What if they both touched/modified a shared catalog?
I was assuming that that would already skip all predicate locking because:
/*
* Does this relation participate in predicate locking? Temporary and system
* relations are exempt.
*/
static inline bool
PredicateLockingNeededForRelation(Relation relation)
{
return !(relation->rd_id < FirstUnpinnedObjectId ||
RelationUsesLocalBuffers(relation));
}
If we ever wanted to use SSI on catalogs, or allow shared relations
that aren't catalogs, or allow cross-database access, then this
optimisation wouldn't fly.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:42 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Tom Lane <tgl@sss.pgh.pa.us>
2023-03-07 01:14 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
@ 2023-03-07 01:16 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 12+ messages in thread
From: Tom Lane @ 2023-03-07 01:16 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Thomas Munro <thomas.munro@gmail.com> writes:
> On Tue, Mar 7, 2023 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What if they both touched/modified a shared catalog?
> I was assuming that that would already skip all predicate locking because:
> /*
> * Does this relation participate in predicate locking? Temporary and system
> * relations are exempt.
> */
Oh, I'd forgotten about that.
> If we ever wanted to use SSI on catalogs, or allow shared relations
> that aren't catalogs, or allow cross-database access, then this
> optimisation wouldn't fly.
I suppose there are some gotchas in the way of making that happen,
so okay.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
@ 2023-03-08 11:57 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-08 20:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
1 sibling, 1 reply; 12+ messages in thread
From: Thomas Munro @ 2023-03-08 11:57 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Tue, Mar 7, 2023 at 12:39 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> Scratch that. I like the more ambitious idea better. I originally
> thought it would be less back-patchable, but that's nonsense -- this
> was a straight up thinko in ancient commit bdaabb9b and we should just
> fix it properly. That is, we should treat this case as a
> generalisation of the the PredXact->WritableSxactCount == 0 case, it's
> just that we had to do more work to figure that out. Here's a patch
> like that.
Here's a better version. I plan to push this tomorrow if there are no
objections.
Attachments:
v4-0001-Fix-race-in-SERIALIZABLE-READ-ONLY.patch (text/x-patch, 3.9K) download
From a1191e4353b8f46948cf3abacda93e7f2b9e6a3a Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmunro@postgresql.org>
Date: Wed, 8 Mar 2023 22:56:02 +1300
Subject: [PATCH v4] Fix race in SERIALIZABLE READ ONLY.
Commit bdaabb9b started skipping doomed transactions when building the
list of possible conflicts for SERIALIZABLE READ ONLY. That makes
sense, because doomed transactions won't commit, but a couple of subtle
things broke:
1. If all uncommitted r/w transactions are doomed, a READ ONLY
transaction would arbitrarily not benefit from the safe snapshot
optimization. It would not be taken immediately, and yet no other
transaction would set SXACT_FLAG_RO_SAFE later.
2. In the same circumstances but with DEFERRABLE, GetSafeSnapshot()
would correctly exit its wait loop without sleeping and then take the
optimization in non-assert builds, but assert builds would fail a sanity
check that SXACT_FLAG_RO_SAFE had been set by another transaction.
This is similar to the case for PredXact->WritableSxactCount == 0. We
should opt out immediately if our possibleUnsafeConflicts list is empty
after filtering.
The code to maintain the serializable global xmin is moved down below
the new opt out site, because otherwise we'd have to reverse its effects
before returning.
Back-patch to all supported releases. Bug #17368.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/17116-d6ca217acc180e30%40postgresql.org
Discussion: https://postgr.es/m/20110707212159.GF76634%40csail.mit.edu
---
src/backend/storage/lmgr/predicate.c | 49 ++++++++++++++++++----------
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 6b5a416873..23461b46f6 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1782,24 +1782,6 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
return snapshot;
}
- /* Maintain serializable global xmin info. */
- if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
- {
- Assert(PredXact->SxactGlobalXminCount == 0);
- PredXact->SxactGlobalXmin = snapshot->xmin;
- PredXact->SxactGlobalXminCount = 1;
- SerialSetActiveSerXmin(snapshot->xmin);
- }
- else if (TransactionIdEquals(snapshot->xmin, PredXact->SxactGlobalXmin))
- {
- Assert(PredXact->SxactGlobalXminCount > 0);
- PredXact->SxactGlobalXminCount++;
- }
- else
- {
- Assert(TransactionIdFollows(snapshot->xmin, PredXact->SxactGlobalXmin));
- }
-
/* Initialize the structure. */
sxact->vxid = vxid;
sxact->SeqNo.lastCommitBeforeSnapshot = PredXact->LastSxactCommitSeqNo;
@@ -1839,6 +1821,19 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
SetPossibleUnsafeConflict(sxact, othersxact);
}
}
+
+ /*
+ * If we didn't find any possibly unsafe conflicts because every
+ * uncommitted writable transaction turned out to be doomed, then we
+ * can "opt out" immediately. See comments above the earlier check for
+ * PredXact->WritableSxactCount == 0.
+ */
+ if (dlist_is_empty(&sxact->possibleUnsafeConflicts))
+ {
+ ReleasePredXact(sxact);
+ LWLockRelease(SerializableXactHashLock);
+ return snapshot;
+ }
}
else
{
@@ -1847,6 +1842,24 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
(MaxBackends + max_prepared_xacts));
}
+ /* Maintain serializable global xmin info. */
+ if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ Assert(PredXact->SxactGlobalXminCount == 0);
+ PredXact->SxactGlobalXmin = snapshot->xmin;
+ PredXact->SxactGlobalXminCount = 1;
+ SerialSetActiveSerXmin(snapshot->xmin);
+ }
+ else if (TransactionIdEquals(snapshot->xmin, PredXact->SxactGlobalXmin))
+ {
+ Assert(PredXact->SxactGlobalXminCount > 0);
+ PredXact->SxactGlobalXminCount++;
+ }
+ else
+ {
+ Assert(TransactionIdFollows(snapshot->xmin, PredXact->SxactGlobalXmin));
+ }
+
MySerializableXact = sxact;
MyXactDidWrite = false; /* haven't written anything yet */
--
2.39.2
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-08 11:57 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
@ 2023-03-08 20:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-03-08 20:50 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Alexander Lakhin @ 2023-03-08 20:00 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Hi Thomas,
08.03.2023 14:57, Thomas Munro wrote:
> On Tue, Mar 7, 2023 at 12:39 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>> Scratch that. I like the more ambitious idea better. I originally
>> thought it would be less back-patchable, but that's nonsense -- this
>> was a straight up thinko in ancient commit bdaabb9b and we should just
>> fix it properly. That is, we should treat this case as a
>> generalisation of the the PredXact->WritableSxactCount == 0 case, it's
>> just that we had to do more work to figure that out. Here's a patch
>> like that.
> Here's a better version. I plan to push this tomorrow if there are no
> objections.
Just in case you find a test for the fix useful, maybe the attached
patch could be added to the commit.
(It triggers the assert on unpatched master.)
Best regards,
Alexander
Attachments:
Test-for-race-in-SERIALIZABLE-READ-ONLY.patch (text/x-patch, 2.2K) download
diff --git a/src/test/isolation/expected/read-only-anomaly-3.out b/src/test/isolation/expected/read-only-anomaly-3.out
index 4f7d3f863d..dbaf01944e 100644
--- a/src/test/isolation/expected/read-only-anomaly-3.out
+++ b/src/test/isolation/expected/read-only-anomaly-3.out
@@ -1,6 +1,6 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 4 sessions
-starting permutation: s2rx s2ry s1ry s1wy s1c s3r s2wx s2c s3c
+starting permutation: s2rx s2ry s1ry s1wy s1c s3r s2wx s2c s3c s4c
step s2rx: SELECT balance FROM bank_account WHERE id = 'X';
balance
-------
@@ -32,3 +32,30 @@ Y | 20
(2 rows)
step s3c: COMMIT;
+step s4c: COMMIT;
+
+starting permutation: s2ry s1wy s1c s2wx s4to s4wx s4c s3r s2c s3c
+step s2ry: SELECT balance FROM bank_account WHERE id = 'Y';
+balance
+-------
+ 0
+(1 row)
+
+step s1wy: UPDATE bank_account SET balance = 20 WHERE id = 'Y';
+step s1c: COMMIT;
+step s2wx: UPDATE bank_account SET balance = -11 WHERE id = 'X';
+step s4to: SET lock_timeout = '10ms'
+step s4wx: UPDATE bank_account SET balance = -11 WHERE id = 'X'; <waiting ...>
+step s4wx: <... completed>
+ERROR: canceling statement due to lock timeout
+step s4c: COMMIT;
+step s3r: SELECT id, balance FROM bank_account WHERE id IN ('X', 'Y') ORDER BY id;
+id|balance
+--+-------
+X | 0
+Y | 20
+(2 rows)
+
+step s2c: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step s3c: COMMIT;
diff --git a/src/test/isolation/specs/read-only-anomaly-3.spec b/src/test/isolation/specs/read-only-anomaly-3.spec
index 61d9c0b9e0..626b66a15a 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; }
-permutation s2rx s2ry s1ry s1wy s1c s3r s2wx s2c s3c
+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 s4c
+permutation s2ry s1wy s1c s2wx s4to s4wx s4c s3r s2c s3c
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-08 11:57 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-08 20:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
@ 2023-03-08 20:50 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-09 04:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Munro @ 2023-03-08 20:50 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Thu, Mar 9, 2023 at 9:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> Just in case you find a test for the fix useful, maybe the attached
> patch could be added to the commit.
> (It triggers the assert on unpatched master.)
I was wondering about that. I like the test and I was using exactly
that + also some other techniques while working on the fix, but the
point of that particular test spec is to show a variation of the
standard example from the read-only-anomaly paper. Hmm, yeah, that's
not a problem, I can just add the extra schedule with a comment to
explain. Will do. Thanks!
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-08 11:57 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-08 20:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-08 20:50 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
@ 2023-03-09 04:39 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-09 09:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Munro @ 2023-03-09 04:39 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Pushed, without that test.
I realised that the test would not be stable in the build farm. If we
made the lock timeout high, the test would be slow, but if we made it
low, then there would be two possible outputs depending on a race, and
10ms as you had it seems -- I guess? -- likely to be unstable under
valgrind or an RPi2 or something. There is probably some clever way
to write a different test schedule, but the new code is exercised by
existing tests, and the assertion has been failing once every couple
of days on CI since I started collecting that data a few weeks ago, so
we have some kind of coverage, at least for master.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-08 11:57 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-08 20:00 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Alexander Lakhin <exclusion@gmail.com>
2023-03-08 20:50 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
2023-03-09 04:39 ` Re: BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction Thomas Munro <thomas.munro@gmail.com>
@ 2023-03-09 09:00 ` Alexander Lakhin <exclusion@gmail.com>
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Lakhin @ 2023-03-09 09:00 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
09.03.2023 07:39, Thomas Munro wrote:
> Pushed, without that test.
>
> I realised that the test would not be stable in the build farm. If we
> made the lock timeout high, the test would be slow, but if we made it
> low, then there would be two possible outputs depending on a race, and
> 10ms as you had it seems -- I guess? -- likely to be unstable under
> valgrind or an RPi2 or something. There is probably some clever way
> to write a different test schedule, but the new code is exercised by
> existing tests, and the assertion has been failing once every couple
> of days on CI since I started collecting that data a few weeks ago, so
> we have some kind of coverage, at least for master.
I had thought that we can use the same timeout that can be seen in
a couple of other tests, but now I see that those tests don't depend
on it directly/have outweighing timeouts, so I completely agree, that
the test proposed is not elaborated enough to be committed.
Thank you!
Best regards,
Alexander
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2023-03-09 09:00 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-01-15 08:00 BUG #17368: Assert failed in GetSafeSnapshot() for SERIALIZABLE READ ONLY DEFERRABLE transaction PG Bug reporting form <noreply@postgresql.org>
2022-01-15 16:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-03-06 03:02 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:39 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-06 23:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-07 01:14 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-07 01:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-08 11:57 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-08 20:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-03-08 20:50 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-09 04:39 ` Thomas Munro <thomas.munro@gmail.com>
2023-03-09 09:00 ` Alexander Lakhin <exclusion@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox