Horton

Thread

Commits

  1. Fix one-off bug causing missing commit timestamps for subtransactions

  1. Null commitTS bug

    Kingsborough, Alex <kingsboa@amazon.com> — 2022-01-14T22:49:59Z

    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] 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] 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

  2. Re: Null commitTS bug

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-17T02:17:24Z

    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

  3. Re: Null commitTS bug

    Michael Paquier <michael@paquier.xyz> — 2022-01-17T03:45:35Z

    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

  4. Re: Null commitTS bug

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-17T04:01:25Z

    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

  5. Re: Null commitTS bug

    Michael Paquier <michael@paquier.xyz> — 2022-01-18T01:43:55Z

    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

  6. Re: Null commitTS bug

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-18T04:48:11Z

    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

  7. Re: Null commitTS bug

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-20T03:00:56Z

    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

  8. Re: Null commitTS bug

    Michael Paquier <michael@paquier.xyz> — 2022-01-20T10:50:31Z

    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

  9. Re: Null commitTS bug

    Michael Paquier <michael@paquier.xyz> — 2022-01-21T06:21:58Z

    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