Обсуждение: Small review comment on pg_checksums

Hi,

While reading the pg_checksums code I found the following comment
"Check if cluster is running" is not placed at right place.

    /* Check if cluster is running */
    ControlFile = get_controlfile(DataDir, &crc_ok);
    if (!crc_ok)
    {
        pg_log_error("pg_control CRC value is incorrect");
        exit(1);
    }

    if (ControlFile->pg_control_version != PG_CONTROL_VERSION)
    {
        pg_log_error("cluster is not compatible with this version of
pg_checksums");
        exit(1);
    }

    if (ControlFile->blcksz != BLCKSZ)
    {
        pg_log_error("database cluster is not compatible");
        fprintf(stderr, _("The database cluster was initialized with
block size %u, but pg_checksums was compiled with block size %u.\n"),
                ControlFile->blcksz, BLCKSZ);
        exit(1);
    }

    if (ControlFile->state != DB_SHUTDOWNED &&
        ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
    {
        pg_log_error("cluster must be shut down");
        exit(1);
    }

So I'd like to propose a small fix for that; move the comment to the
right place and add another comment. Please find an attached small
patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Jun 06, 2019 at 05:16:30PM +0900, Masahiko Sawada wrote:
> So I'd like to propose a small fix for that; move the comment to the
> right place and add another comment. Please find an attached small
> patch.

No objections to that.  Perhaps we should also mention that this does
not protect from someone starting the cluster concurrently and that
the reason why we require a clean shutdown is that we may get checksum
failures because of torn pages?
--
Michael

On Thu, Jun 6, 2019 at 10:21 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jun 06, 2019 at 05:16:30PM +0900, Masahiko Sawada wrote:
> > So I'd like to propose a small fix for that; move the comment to the
> > right place and add another comment. Please find an attached small
> > patch.
>
> No objections to that.  Perhaps we should also mention that this does
> not protect from someone starting the cluster concurrently and that
> the reason why we require a clean shutdown is that we may get checksum
> failures because of torn pages?

Agreed. Please find an attached patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Jun 07, 2019 at 03:30:35PM +0900, Masahiko Sawada wrote:
> Agreed. Please find an attached patch.

Thanks, committed.
--
Michael

On Fri, Jun 7, 2019 at 8:52 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jun 07, 2019 at 03:30:35PM +0900, Masahiko Sawada wrote:
> > Agreed. Please find an attached patch.
>
> Thanks, committed.

Thank you!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center