Horton

Thread

Commits

  1. Update outdated comment for TransactionIdSetTreeStatus

  2. Rework subtransaction commit protocol for hot standby.

  1. Confused about TransactionIdSetTreeStatus

    Japin Li <japinli@hotmail.com> — 2022-10-25T09:02:25Z

    Hi, hackers

I'm a bit confused about TransactionIdSetTreeStatus, the comment says if
subtransactions cross multiple CLOG pages, it will mark the subxids, that
are on the same page as the main transaction, as sub-committed, and then
set main transaction and subtransactions to committed (step 2).

 * Example:
 *      TransactionId t commits and has subxids t1, t2, t3, t4
 *      t is on page p1, t1 is also on p1, t2 and t3 are on p2, t4 is on p3
 *      1. update pages2-3:
 *                  page2: set t2,t3 as sub-committed
 *                  page3: set t4 as sub-committed
 *      2. update page1:
 *                  set t1 as sub-committed,
 *                  then set t as committed,
                    then set t1 as committed
 *      3. update pages2-3:
 *                  page2: set t2,t3 as committed
 *                  page3: set t4 as committed

However, the code marks the main transaction and subtransactions directly
to the committed.

        /*
         * If this is a commit then we care about doing this correctly (i.e.
         * using the subcommitted intermediate status).  By here, we know
         * we're updating more than one page of clog, so we must mark entries
         * that are *not* on the first page so that they show as subcommitted
         * before we then return to update the status to fully committed.
         *
         * To avoid touching the first page twice, skip marking subcommitted
         * for the subxids on that first page.
         */
        if (status == TRANSACTION_STATUS_COMMITTED)
            set_status_by_pages(nsubxids - nsubxids_on_first_page,
                                subxids + nsubxids_on_first_page,
                                TRANSACTION_STATUS_SUB_COMMITTED, lsn);

        /*
         * Now set the parent and subtransactions on same page as the parent,
         * if any
         */
        pageno = TransactionIdToPage(xid);
        TransactionIdSetPageStatus(xid, nsubxids_on_first_page, subxids, status,
                                   lsn, pageno, false);

        /*
         * Now work through the rest of the subxids one clog page at a time,
         * starting from the second page onwards, like we did above.
         */
        set_status_by_pages(nsubxids - nsubxids_on_first_page,
                            subxids + nsubxids_on_first_page,
                            status, lsn);

Is the comment correct?  If not, should we remove it?

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

  2. Re: Confused about TransactionIdSetTreeStatus

    Heikki Linnakangas <hlinnaka@iki.fi> — 2022-10-25T14:46:49Z

    On 25/10/2022 12:02, Japin Li wrote:
> I'm a bit confused about TransactionIdSetTreeStatus, the comment says if
> subtransactions cross multiple CLOG pages, it will mark the subxids, that
> are on the same page as the main transaction, as sub-committed, and then
> set main transaction and subtransactions to committed (step 2).
> 
>   * Example:
>   *      TransactionId t commits and has subxids t1, t2, t3, t4
>   *      t is on page p1, t1 is also on p1, t2 and t3 are on p2, t4 is on p3
>   *      1. update pages2-3:
>   *                  page2: set t2,t3 as sub-committed
>   *                  page3: set t4 as sub-committed
>   *      2. update page1:
>   *                  set t1 as sub-committed,
>   *                  then set t as committed,
>                      then set t1 as committed
>   *      3. update pages2-3:
>   *                  page2: set t2,t3 as committed
>   *                  page3: set t4 as committed
> 
> However, the code marks the main transaction and subtransactions directly
> to the committed.

Hmm, yeah, step 2 in this example doesn't match reality. We actually set 
t and t1 directly as committed. The explanation above that comment is 
correct, but the example is not. It used to work the way the example 
says, but that was changed in commit 
06da3c570f21394003fc392d80f54862f7dec19f. Ironically, that commit also 
added the outdated comment.

The correct example would be:

TransactionId t commits and has subxids t1, t2, t3, t4 t is on page p1, 
t1 is also on p1, t2 and t3 are on p2, t4 is on p3
1. update pages2-3:
       page2: set t2,t3 as sub-committed
       page3: set t4 as sub-committed
2. update page1:
       page1: set t,t1 as committed,
3. update pages2-3:
       page2: set t2,t3 as committed
       page3: set t4 as committed

- Heikki

  3. Re: Confused about TransactionIdSetTreeStatus

    Japin Li <japinli@hotmail.com> — 2022-10-25T15:09:03Z

    On Tue, 25 Oct 2022 at 22:46, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 25/10/2022 12:02, Japin Li wrote:
>> However, the code marks the main transaction and subtransactions directly
>> to the committed.
>
> Hmm, yeah, step 2 in this example doesn't match reality. We actually
> set t and t1 directly as committed. The explanation above that comment
> is correct, but the example is not. It used to work the way the
> example says, but that was changed in commit
> 06da3c570f21394003fc392d80f54862f7dec19f. Ironically, that commit also
> added the outdated comment.
>
> The correct example would be:
>
> TransactionId t commits and has subxids t1, t2, t3, t4 t is on page
> p1, t1 is also on p1, t2 and t3 are on p2, t4 is on p3
> 1. update pages2-3:
>       page2: set t2,t3 as sub-committed
>       page3: set t4 as sub-committed
> 2. update page1:
>       page1: set t,t1 as committed,
> 3. update pages2-3:
>       page2: set t2,t3 as committed
>       page3: set t4 as committed
>

Thanks for your explanation.  Attach a patch to remove the outdated comment.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

  4. Re: Confused about TransactionIdSetTreeStatus

    Heikki Linnakangas <hlinnaka@iki.fi> — 2022-10-25T19:44:46Z

    On 25/10/2022 18:09, Japin Li wrote:
> 
> On Tue, 25 Oct 2022 at 22:46, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> On 25/10/2022 12:02, Japin Li wrote:
>>> However, the code marks the main transaction and subtransactions directly
>>> to the committed.
>>
>> Hmm, yeah, step 2 in this example doesn't match reality. We actually
>> set t and t1 directly as committed. The explanation above that comment
>> is correct, but the example is not. It used to work the way the
>> example says, but that was changed in commit
>> 06da3c570f21394003fc392d80f54862f7dec19f. Ironically, that commit also
>> added the outdated comment.
>>
>> The correct example would be:
>>
>> TransactionId t commits and has subxids t1, t2, t3, t4 t is on page
>> p1, t1 is also on p1, t2 and t3 are on p2, t4 is on p3
>> 1. update pages2-3:
>>        page2: set t2,t3 as sub-committed
>>        page3: set t4 as sub-committed
>> 2. update page1:
>>        page1: set t,t1 as committed,
>> 3. update pages2-3:
>>        page2: set t2,t3 as committed
>>        page3: set t4 as committed
> 
> Thanks for your explanation.  Attach a patch to remove the outdated comment.

Applied, thanks!

- Heikki