Обсуждение: BUG #3143: MyLastRecPtr.xlogid not updated with MyLastRecPtr.xrecoff?

The following bug has been logged online:

Bug reference:      3143
Logged by:          Chongfeng Hu
Email address:      loveminix@yahoo.com.cn
PostgreSQL version: 8.2.3
Operating system:   any
Description:        MyLastRecPtr.xlogid not updated with
MyLastRecPtr.xrecoff?
Details:

I checked the source of PostgreSQL, and found one suspicious spot in file
src/backend/access/transam/xact.c. In struct XLogRecPtr, it has two fields:
xlogid, indicating log file #, and xrecoff, indicating the byte offset of
location in log file, so they should always keep consistent, as I saw in
many places, just list one of them:

        if (tmpRecPtr.xrecoff >= XLogFileSize)
        {
            (tmpRecPtr.xlogid)++;
            tmpRecPtr.xrecoff = 0;
        }

However, in file src/backend/access/transam/xact.c, on line 1778, I saw the
following code:

    /* Break the chain of back-links in the XLOG records I output */
    MyLastRecPtr.xrecoff = 0;
    MyXactMadeXLogEntry = false;
    MyXactMadeTempRelUpdate = false;

It only updated xrecoff, but not xlogid. This might cause serious problems,
because xlogid is still pointing to an old file, while xrecoff is pointing
to a new offset. It might read incorrect records.

Chongfeng Hu wrote:
> However, in file src/backend/access/transam/xact.c, on line 1778, I saw the
> following code:
>
>     /* Break the chain of back-links in the XLOG records I output */
>     MyLastRecPtr.xrecoff = 0;
>     MyXactMadeXLogEntry = false;
>     MyXactMadeTempRelUpdate = false;
>
> It only updated xrecoff, but not xlogid. This might cause serious problems,
> because xlogid is still pointing to an old file, while xrecoff is pointing
> to a new offset. It might read incorrect records.

No, that's actually ok. MyLastRecPtr is used to check if the current
transaction has made any transaction-controlled WAL records. If
MyLastRecPtr.xrecoff == 0, it hasn't, otherwise it has. All readers of
the variable just check for MyLastRecPtr.xrecoff == 0, so the above is OK.

I agree that it is a bit misleading, though. A boolean
"MyXactMadeTransactionControlledXlogEntry", like the
"MyXactMadeXLogEntry" and "MyXactMadeTempRelupdate" variables would be
cleaner.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki@enterprisedb.com> writes:
> I agree that it is a bit misleading, though. A boolean
> "MyXactMadeTransactionControlledXlogEntry", like the
> "MyXactMadeXLogEntry" and "MyXactMadeTempRelupdate" variables would be
> cleaner.

I think at one time the back-link nature of the variable was actually
exploited --- it was used to make an additional back-link in XLOG record
headers.  But it's true that it isn't much more than a bool at present.

            regards, tom lane

Tom Lane wrote:
> Heikki Linnakangas <heikki@enterprisedb.com> writes:
> > I agree that it is a bit misleading, though. A boolean
> > "MyXactMadeTransactionControlledXlogEntry", like the
> > "MyXactMadeXLogEntry" and "MyXactMadeTempRelupdate" variables would be
> > cleaner.
>
> I think at one time the back-link nature of the variable was actually
> exploited --- it was used to make an additional back-link in XLOG record
> headers.  But it's true that it isn't much more than a bool at present.

Should someone clean up that variable usage?

--
  Bruce Momjian  <bruce@momjian.us>          http://momjian.us
  EnterpriseDB                               http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +