MIME-Version: 1.0
References: <20221123014224.xisi44byq3cf5psi@awork3.anarazel.de>
 <CA+hUKGL9w0VhZ=ceMDjXgCXy1BC5VzjwXc-HetSxfNzvBLge3A@mail.gmail.com>
 <CA+hUKGKr0TBwFMZ_BxcOps4_WDxhX5tS2D1Mwb1fAHyMKxo-UA@mail.gmail.com>
 <3748783.1669241103@sss.pgh.pa.us> <CA+hUKGK-BEe38aKNqHJDQ86LUW-CMwF5F9bo1JtJVg71FoDv_w@mail.gmail.com>
 <20d45197-80be-4984-33a9-a8d13708992a@inbox.ru>
In-Reply-To: <20d45197-80be-4984-33a9-a8d13708992a@inbox.ru>
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 31 Jan 2023 17:09:05 +1300
Message-ID: <CA+hUKGKzODWydEc0ZyAYXyFscqSQAvxDWCkDEOggiP4OZdr9BQ@mail.gmail.com>
Subject: Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
To: "Anton A. Melnikov" <aamelnikov@inbox.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CA%2BhUKGKzODWydEc0ZyAYXyFscqSQAvxDWCkDEOggiP4OZdr9BQ%40mail.gmail.com>
Precedence: bulk

On Tue, Jan 31, 2023 at 2:10 PM Anton A. Melnikov <aamelnikov@inbox.ru> wrote:
> Also checked for a few hours that the patch 0002 fixes this error,
> but there are some questions to its logical structure.

Hi Anton,

Thanks for looking!

> The equality between the previous and newly calculated crc is checked only
> if the last crc calculation was wrong, i.e not equal to the value stored in the file.
> It is very unlikely that in this case the previous and new crc can match, so, in fact,
> the loop will spin until crc is calculated correctly. In the other words,
> this algorithm is practically equivalent to an infinite loop of reading from a file
> and calculating crc while(EQ_CRC32C(crc, ControlFile->crc) != true).

Maybe it's unlikely that two samples will match while running that
torture test, because it's overwriting the file as fast as it can.
But the idea is that a real system isn't writing the control file most
of the time.

> But then it can be simplified significantly by removing checksums equality checks,
> bool fist_try and by limiting the maximum number of iterations
> with some constant in the e.g. for loop to avoid theoretically possible freeze.

Yeah, I was thinking that we should also put a limit on the loop, just
to be cautious.

Primary servers write the control file very infrequently.  Standybys
more frequently, while writing data out, maybe every few seconds on a
busy system writing out a lot of data.  UpdateMinRecoveryPoint() makes
some effort to avoid updating the file too often.  You definitely see
bursts of repeated flushes that might send this thing in a loop for a
while if the timings were exactly wrong, but usually with periodic
gaps; I haven't really studied the expected behaviour too closely.

> Or maybe use the variant suggested by Tom Lane, i.e, as far as i understand,
> repeat the file_open-read-close-calculate_crc sequence twice without a pause between
> them and check the both calculated crcs for the equality. If they match, exit and return
> the bool result of comparing between the last calculation with the value from the file,
> if not, take a pause and repeat everything from the beginning.

Hmm.  Would it be good enough to do two read() calls with no sleep in
between?  How sure are we that a concurrent write will manage to
change at least one bit that our second read can see?  I guess it's
likely, but maybe hypervisors, preemptible kernels, I/O interrupts or
a glitch in the matrix could decrease our luck?  I really don't know.
So I figured I should add a small sleep between the reads to change
the odds in our favour.  But we don't want to slow down all reads of
the control file with a sleep, do we?  So I thought we should only
bother doing this slow stuff if the actual CRC check fails, a low
probability event.

Clearly there is an element of speculation or superstition here.  I
don't know what else to do if both PostgreSQL and ext4 decided not to
add interlocking.  Maybe we should rethink that.  How bad would it
really be if control file access used POSIX file locking?  I mean, the
writer is going to *fsync* the file, so it's not like one more wafer
thin system call is going to hurt too much.