Null commitTS bug
Hi Hackers,
I've been working with commitTS code recently and during my testing I
found a bug when writing commit timestamps for subxids. Normally for
sub-transaction commit timestamps in TransactionTreeSetCommitTsData(),
we iterate through the subxids until we find one that is on the next commits
page. In the code [1] https://github.com/postgres/postgres/blame/master/src/backend/access/transam/commit_ts.c#L178 this is the jth subxid. In SetXidCommitTsInPage()
where we set all the commit timestamps up to but not including the jth
timestamp. The jth timestamp then becomes the head timestamp for next
group of timestamps on the next page. However, if the jth timestamp is
the last subxid (put another way, if the LAST subxid is the FIRST
timestamp on a new page), then the code will break on line 188 [2] https://github.com/postgres/postgres/blame/master/src/backend/access/transam/commit_ts.c#L188 and
the timestamp for the last subxid will never be written.
This can be reproduced by enabling track_commit_timestamp and running
a simple loop that has a single sub-transaction like:
psql -t -c 'create table t (id int);'
for i in {1..500}
do
psql -t -c 'begin; insert into t select 1; savepoint a; insert into t select 2;commit'
done
Then querying for NULL commitTS in that table will return that there are
unwritten timestamps:
postgres=# select count(*) from t where pg_xact_commit_timestamp(t.xmin) is NULL;
count
1
(1 row)
The fix for this is very simple
/* if we wrote out all subxids, we're done. /
- if (j + 1 >= nsubxids)
+ if (j >= nsubxids)
break;
[1]: https://github.com/postgres/postgres/blame/master/src/backend/access/transam/commit_ts.c#L178
[2]: https://github.com/postgres/postgres/blame/master/src/backend/access/transam/commit_ts.c#L188
Attachments:
0001-commitTS-subxids-bug-fix.patchapplication/octet-stream; name=0001-commitTS-subxids-bug-fix.patchDownload
From e49a60353441deffd45dafbd83c7cecf90e24689 Mon Sep 17 00:00:00 2001
From: Alex Kingsborough <kingsboa@amazon.com>
Date: Fri, 14 Jan 2022 21:32:25 +0000
Subject: [PATCH] commitTS subxids bug fix
In commitTS if the last subxid was the first timestamp on a page, that
timestamp would not get written, this change makes sure we write all the
subxid timestamps correctly
---
src/backend/access/transam/commit_ts.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 0985fa155c..3d32507345 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -183,7 +183,7 @@ TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
pageno);
/* if we wrote out all subxids, we're done. */
- if (j + 1 >= nsubxids)
+ if (j >= nsubxids)
break;
/*
--
2.16.6
At Fri, 14 Jan 2022 22:49:59 +0000, "Kingsborough, Alex" <kingsboa@amazon.com> wrote in
The fix for this is very simple
/* if we wrote out all subxids, we're done. / - if (j + 1 >= nsubxids) + if (j >= nsubxids) break;
It looks like a thinko and the fix is correct. (It's a matter of taste
choosing between it and "j == nsubxids").
I found some confusing lines around but they need not a fix
considering back-patching conflict?
for (i = 0, headxid = xid;;)
..
i += j - i + 1;
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Mon, Jan 17, 2022 at 11:17:24AM +0900, Kyotaro Horiguchi wrote:
At Fri, 14 Jan 2022 22:49:59 +0000, "Kingsborough, Alex" <kingsboa@amazon.com> wrote in
The fix for this is very simple
/* if we wrote out all subxids, we're done. / - if (j + 1 >= nsubxids) + if (j >= nsubxids) break;It looks like a thinko and the fix is correct. (It's a matter of taste
choosing between it and "j == nsubxids").
It took me some time to understand the problem from the current code,
but I'd like to think that the suggested fix is less confusing.
I found some confusing lines around but they need not a fix
considering back-patching conflict?for (i = 0, headxid = xid;;)
..
i += j - i + 1;
I am not sure. Do you have anything specific in mind? Perhaps
something that would help in making the code logic easier to follow?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Jan 17, 2022 at 11:17:24AM +0900, Kyotaro Horiguchi wrote:
I found some confusing lines around but they need not a fix
considering back-patching conflict?i += j - i + 1;
I am not sure. Do you have anything specific in mind? Perhaps
something that would help in making the code logic easier to follow?
Isn't that a very bad way to write "i = j + 1"?
I agree with Horiguchi-san that
for (i = 0, headxid = xid;;)
is not great style either. A for-loop ought to be used to control the
number of iterations, not as a confusing variable initialization.
I think more idiomatic would be
headxid = xid;
i = 0;
for (;;)
which makes it clear that this is not where the loop control is.
regards, tom lane
On Sun, Jan 16, 2022 at 11:01:25PM -0500, Tom Lane wrote:
Isn't that a very bad way to write "i = j + 1"?
I agree with Horiguchi-san that
for (i = 0, headxid = xid;;)
Okay. Horiguchi-san, would you like to write a patch?
--
Michael
At Tue, 18 Jan 2022 10:43:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Sun, Jan 16, 2022 at 11:01:25PM -0500, Tom Lane wrote:
Isn't that a very bad way to write "i = j + 1"?
I agree with Horiguchi-san that
for (i = 0, headxid = xid;;)Okay. Horiguchi-san, would you like to write a patch?
Yes, I will.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Tue, 18 Jan 2022 13:48:11 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
At Tue, 18 Jan 2022 10:43:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Sun, Jan 16, 2022 at 11:01:25PM -0500, Tom Lane wrote:
Isn't that a very bad way to write "i = j + 1"?
I agree with Horiguchi-san that
for (i = 0, headxid = xid;;)Okay. Horiguchi-san, would you like to write a patch?
Yes, I will.
This is that. I think this is a separate issue from the actual
bug. This is applicable at least back to 9.6 and I think this should
be applied back to all supported versions to avoid future backptach
conflicts.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Improve-confusing-code-in-TransactionTreeSetCommitTs.txttext/plain; charset=us-asciiDownload
From 9211fa61513dcdbca273656454395a3dcf3ee4e7 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 20 Jan 2022 10:16:48 +0900
Subject: [PATCH] Improve confusing code in TransactionTreeSetCommitTsData
TransactionTreeSetCommitTsData has a bit confusing use of the +=
operator. Simplifying it makes the code easier to follow. In the
same function for-loop initializes only non-controlling variables,
which is not great style. They ought to be initialized outside the
loop.
---
src/backend/access/transam/commit_ts.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 659109f8d4..88eac10456 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -168,7 +168,10 @@ TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
* subxid not on the previous page as head. This way, we only have to
* lock/modify each SLRU page once.
*/
- for (i = 0, headxid = xid;;)
+ headxid = xid;
+ i = 0;
+
+ for (;;)
{
int pageno = TransactionIdToCTsPage(headxid);
int j;
@@ -192,7 +195,7 @@ TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
* just wrote.
*/
headxid = subxids[j];
- i += j - i + 1;
+ i = j + 1;
}
/* update the cached value in shared memory */
--
2.27.0
On Thu, Jan 20, 2022 at 12:00:56PM +0900, Kyotaro Horiguchi wrote:
This is that. I think this is a separate issue from the actual
bug. This is applicable at least back to 9.6 and I think this should
be applied back to all supported versions to avoid future backptach
conflicts.
Thanks. I'll check that tomorrow.
--
Michael
On Thu, Jan 20, 2022 at 12:00:56PM +0900, Kyotaro Horiguchi wrote:
This is that. I think this is a separate issue from the actual
bug. This is applicable at least back to 9.6 and I think this should
be applied back to all supported versions to avoid future backptach
conflicts.
Looks fine enough. I have grouped that with Alex's fix and
backpatched the whole down to v10. Thanks!
--
Michael