public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bharath Rupireddy <[email protected]>
To: Fujii Masao <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Stephen Frost <[email protected]>
Cc: Bossart, Nathan <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
Date: Wed, 2 Feb 2022 20:16:35 +0530
Message-ID: <CALj2ACVVMAGDvourutDvtDUOhTqS_yTtfrdCjtP75-RrQU0dFA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALj2ACXDHGfhKkwzi81BRqWft4n85AO1zmkeqf6xEg+0xTCH9g@mail.gmail.com>
	<[email protected]>
	<CALj2ACU9ynpCwfuL0a4Bx0SDbHcjRDk49pwjDwfOzoQ1-kx1pw@mail.gmail.com>
	<[email protected]>
	<CALj2ACUtZhTb=2ENkF3BQ3wi137uaGi__qzvXC-qFYC0XwjALw@mail.gmail.com>
	<[email protected]>

On Tue, Feb 1, 2022 at 9:39 PM Fujii Masao <[email protected]> wrote:
> I found that CreateRestartPoint() already reported the redo lsn as follows after emitting the restartpoint log message. To avoid duplicated logging of the same information, we should update this code?
>
>         ereport((log_checkpoints ? LOG : DEBUG2),
>                         (errmsg("recovery restart point at %X/%X",
>                                         LSN_FORMAT_ARGS(lastCheckPoint.redo)),
>                          xtime ? errdetail("Last completed transaction was at log time %s.",
>                                                            timestamptz_to_str(xtime)) : 0));
>
> This code reports lastCheckPoint.redo as redo lsn. OTOH, with the patch, LogCheckpointEnd() reports ControlFile->checkPointCopy.redo. They may be different, for example, when the current DB state is not DB_IN_ARCHIVE_RECOVERY. In this case, which lsn should we report as redo lsn?

Do we ever reach CreateRestartPoint when ControlFile->stat !=
DB_IN_ARCHIVE_RECOVERY? Assert(ControlFile->state ==
DB_IN_ARCHIVE_RECOVERY); in CreateRestartPoint doesn't fail any
regression tests.

Here's what can happen:
lastCheckPoint.redo is 100 and ControlFile->checkPointCopy.redo is
105, so, "skipping restartpoint, already performed at %X/%X"
LogCheckpointEnd isn't reached
lastCheckPoint.redo is 105 and ControlFile->checkPointCopy.redo is 100
and ControlFile->state == DB_IN_ARCHIVE_RECOVERY, then the control
file gets updated and LogCheckpointEnd prints the right redo lsn.
lastCheckPoint.redo is 105 and ControlFile->checkPointCopy.redo is 100
and ControlFile->state != DB_IN_ARCHIVE_RECOVERY, the the control file
doesn't get updated and LogCheckpointEnd just prints the control redo
lsn. Looks like this case is rare given Assert(ControlFile->state ==
DB_IN_ARCHIVE_RECOVERY); doesn't fail any tests.

I think we should just let LogCheckpointEnd print the redo lsn from
the control file. We can just remove the above errmsg("recovery
restart point at %X/%X" message altogether or just print it only in
the rare scenario, something like below:

if (ControlFile->state != DB_IN_ARCHIVE_RECOVERY)
{
    ereport((log_checkpoints ? LOG : DEBUG2),
            (errmsg("performed recovery restart point at %X/%X while
the database state is %s",
                    LSN_FORMAT_ARGS(lastCheckPoint.redo)),
getDBState(ControlFile->state)));
}

And the last commit/abort records's timestamp will always get logged
even before we reach here in the main redo loop (errmsg("last
completed transaction was at log time %s").

Or another way is to just pass the redo lsn to LogCheckpointEnd and
pass the lastCheckPoint.redo in if (ControlFile->state !=
DB_IN_ARCHIVE_RECOVERY) cases or when control file isn't updated but
restart point happened.

Thoughts?

Regards,
Bharath Rupireddy.

view thread (6+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
  In-Reply-To: <CALj2ACVVMAGDvourutDvtDUOhTqS_yTtfrdCjtP75-RrQU0dFA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox