Offline enabling/disabling of data checksums

classic Classic list List threaded Threaded
149 messages Options
123456 ... 8
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3

Hallo Michael,

>> - * src/bin/pg_verify_checksums/pg_verify_checksums.c
>> + * src/bin/pg_checksums/pg_checksums.c
>> That's lacking a rename, or this comment is incorrect.
>
> Right, I started the rename, but then backed off pending further
> discussion whether I should submit that or whether the committer will
> just do it.

ISTM that there is a all clear for renaming.

The renaming implies quite a few changes (eg in the documentation,
makefiles, tests) which warrants a review, so it should be a patch. Also,
ISTM that the renaming only make sense when adding the enable/disable
feature, so I'd say that it belongs to this patch. Opinions?

About v4:

Patch applies cleanly, compiles, global & local "make check" are ok.

Doc: "enable, disable or verifies" -> "checks, enables or disables"?
Spurious space: "> ." -> ">.".

As checksum are now checked, the doc could use "check" instead of
"verify", especially if there is a rename and the "verify" word
disappears.

I'd be less terse when documenting actions, eg: "Disable checksums" ->
"Disable checksums on cluster."

Doc should state that checking is the default action, eg "Check checksums
on cluster. This is the default action."

Help string could say that -c is the default action. There is a spurious
help line remaining from the previous "--action" implementation.

open: I'm positively unsure about ?: priority over |, and probably not the
only one, so I'd add parentheses around the former.

I'm at odds with the "postmaster.pid" check, which would not prevent an
issue if a cluster is started with "postmaster". I still think that the
enabling-in-progress should be stored in the cluster state.

ISTM that the cluster read/update cycle should lock somehow the control
file being modified. However other commands do not seem to do something
about it.

I do not think that enabling if already enabled or disabling or already
disable should exit(1), I think it is a no-op and should simply exit(0).

About tests: I'd run a check on a disabled cluster to check that the
command fails because disabled.

--
Fabien.

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
On Wed, Feb 27, 2019 at 07:59:31AM +0100, Fabien COELHO wrote:
> The renaming implies quite a few changes (eg in the documentation,
> makefiles, tests) which warrants a review, so it should be a patch. Also,
> ISTM that the renaming only make sense when adding the enable/disable
> feature, so I'd say that it belongs to this patch. Opinions?

I would think that the rename should happen first, but it is possible
to make git diffs less noisy as well for files copied, so merging
things is technically doable.

> About tests: I'd run a check on a disabled cluster to check that the command
> fails because disabled.

While I look at that...  If you could split the refactoring into a
separate, first, patch as well..
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
In reply to this post by Fabien COELHO-3
On Wed, Feb 27, 2019 at 07:59:31AM +0100, Fabien COELHO wrote:
> Hallo Michael,

Okay, let's move on with these patches!

> The renaming implies quite a few changes (eg in the documentation,
> makefiles, tests) which warrants a review, so it should be a patch. Also,
> ISTM that the renaming only make sense when adding the enable/disable
> feature, so I'd say that it belongs to this patch. Opinions?

I have worked on the last v4 sent by Michael B, finishing with the
attached after review and addressed the last points raised by Fabien.
The thing is that I have been rather unhappy with a couple of things
in what was proposed, so I have finished by modifying quite a couple
of areas.  The patch set is now splitted as I think is suited for
commit, with the refactoring and renaming being separated from the
actual feature:
- 0001 if a patch to refactor the routine for the control file
update.  I have made it backend-aware, and we ought to be careful with
error handling, use of fds and such, something that v4 was not very
careful about.
- 0002 renames pg_verify_checksums to pg_checksums with a
straight-forward switch.  Docs as well as all references to
pg_verify_checksums are updated.
- 0003 adds the new options --check, --enable and --disable, with
--check being the default as discussed.
- 0004 adds a -N/--no-sync which I think is nice for consistency with
other tools.  That's also useful for the tests, and was not discussed
until now on this thread.

> Patch applies cleanly, compiles, global & local "make check" are ok.
>
> Doc: "enable, disable or verifies" -> "checks, enables or disables"?
> Spurious space: "> ." -> ">.".
>
> As checksum are now checked, the doc could use "check" instead of "verify",
> especially if there is a rename and the "verify" word disappears.

That makes sense.  I have fixed these, and simplified the docs a bit
to have a more simple page.

> I'd be less terse when documenting actions, eg: "Disable checksums" ->
> "Disable checksums on cluster."

The former is fine in my opinion.

> Doc should state that checking is the default action, eg "Check checksums on
> cluster. This is the default action."

Check.

> Help string could say that -c is the default action. There is a spurious
> help line remaining from the previous "--action" implementation.

Yeah, we should.  Added.

> open: I'm positively unsure about ?: priority over |, and probably not the
> only one, so I'd add parentheses around the former.

Yes, I agree that the current code is hard to decrypt.  So reworked
with a separate variable in scan_file, and added extra parenthesis in
the part which updates the control file.

> I'm at odds with the "postmaster.pid" check, which would not prevent an
> issue if a cluster is started with "postmaster". I still think that the
> enabling-in-progress should be stored in the cluster state.
>
> ISTM that the cluster read/update cycle should lock somehow the control file
> being modified. However other commands do not seem to do something about it.

I am still not on board for adding more complexity in this area, at
least not for this stuff and for the purpose of this thread, because
this can happen at various degrees for various configurations for ages
and not only for checksums.  Also, I think that we still have to see
users complain about that.  Here are some scenarios where this can
happen:
- A base backup partially written.  pg_basebackup limits this risk but
it could still be possible to see a case where partially-written data
folder.  And base backups are around for many years.
- pg_rewind, and the tool is in the tree since 9.5, the tool is
actually available on github since 9.3.

> I do not think that enabling if already enabled or disabling or already
> disable should exit(1), I think it is a no-op and should simply exit(0).

We already issue exit(1) when attempting to verify checksums on a
cluster where they are disabled.  So I agree with Michael B's point of
Issuing an error in such cases.  I think also that this makes handling
for operators easier.

> About tests: I'd run a check on a disabled cluster to check that the command
> fails because disabled.

Makes sense.  Added.  We need a test also for the case of successive
runs with --enable.

Here are also some notes from my side.
- There was no need to complicate the synopsis of the docs.
- usage() included still references to --action and indentation was a
bit too long at the top.
- There were no tests for disabling checksums, so I have added some.
- We should check that the combination of --enable and -r fails.
- Tests use only long options, that's better for readability.
- Improved comments in tests.
- Better to check for "data_checksum_version > 0" if --enable is
used.  That's more portable long-term if more checksum versions are
added.
- The check on postmaster.pid is actually not necessary as we already
know that the cluster has been shutdown cleanly per the state of the
control file.  I agree that there could be a small race condition
here, and we could discuss that in a different thread if need be as
such things could be improved for other frontend tools as well.  For
now I am taking the most simple approach.

(Still need to indent the patches before commit, but that's a nit.)
--
Michael

0001-Refactor-routine-for-update-of-control-file.patch (7K) Download Attachment
0002-Rename-pg_verify_checksums-to-pg_checksums.patch (17K) Download Attachment
0003-Add-options-to-enable-and-disable-checksums-in-pg_ch.patch (18K) Download Attachment
0004-Add-option-N-no-sync-to-pg_checksums.patch (6K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Banck-2
Hi Michael,

Am Montag, den 11.03.2019, 13:53 +0900 schrieb Michael Paquier:
> On Wed, Feb 27, 2019 at 07:59:31AM +0100, Fabien COELHO wrote:
> > Hallo Michael,
>
> Okay, let's move on with these patches!

Wow cool. I was going to go back to these and split them up similar to
how you did it now that the online verification patch seems to be
done/stuck for v12, but great that you beat me to it.

I had a quick look over the patch and your changes and it LGTM.


Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: [hidden email]

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Banck-2
Hi,

Am Montag, den 11.03.2019, 11:11 +0100 schrieb Michael Banck:
> I had a quick look over the patch and your changes and it LGTM.

One thing: you (Michael) should be co-author for patch #3 as I took some
of your code from https://github.com/michaelpq/pg_plugins/tree/master/pg
_checksums


Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: [hidden email]

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Sergei Kornilov
In reply to this post by Michael Paquier-2
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

Hello

I review latest patchset. I have one big question: Is pg_checksums safe for cross-versions operations? Even with update_controlfile call? Currently i am able to enable checksums in pg11 cluster with pg_checksums compiled on HEAD. Is this expected? I didn't notice any version-specific check in code.

And few small notes:

> <command>pg_checksums</command> checks, enables or disables data checksums

maybe better is <application>, not <command>?

> + printf(_("%s enables, disables or verifies data checksums in a PostgreSQL\n"), progname);
> + printf(_("database cluster.\n\n"));

I doubt this is good line formatting for translation purposes.

> + printf(_("  -c, --check            check data checksums.  This is the default\n"));
> + printf(_("                         mode if nothing is specified.\n"));

same. For example pg_basebackup uses different multiline style:

> printf(_("  -r, --max-rate=RATE    maximum transfer rate to transfer data directory\n"
> "                         (in kB/s, or use suffix \"k\" or \"M\")\n"));

> +command_fails(['pg_checksums', '--enable', '-r', '1234', '-D', $pgdata],
> +      "fails when relfilnodes are requested and action is --disable");

action is "--enable" here ;-)

> if (badblocks > 0)
> return 1;

Small question: why return 1 instead of exit(1)?

> <refentry id="pgchecksums">
>  <indexterm zone="pgchecksums">

How about use "app-pgchecksums" similar to other applications?

regards, Sergei
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
On Mon, Mar 11, 2019 at 02:11:11PM +0000, Sergei Kornilov wrote:
> I review latest patchset.

Thanks, I have committed the refactoring of src/common/ as a first
step.

> I have one big question: Is pg_checksums
> safe for cross-versions operations? Even with update_controlfile
> call? Currently i am able to enable checksums in pg11 cluster with
> pg_checksums compiled on HEAD. Is this expected? I didn't notice any
> version-specific check in code.

This depends on the version of the control file, and it happens that
we don't check for it, so that's a good catch from your side.  Not
doing the check is a bad idea as ControlFileData should be compatible
between the binary and the data read.  I am attaching a fresh 0001
which should be back-patched down to v11 as a bug fix.  An advantage
of that, which is similar to pg_rewind, is that if the control file
version does not change in a major version, then the tool can be
used.  And the data folder layer is unlikely going to change..

>> <command>pg_checksums</command> checks, enables or disables data checksums
>
> maybe better is <application>, not <command>?

Fixed, as part of the renaming patch.

>> + printf(_("%s enables, disables or verifies data checksums in a PostgreSQL\n"), progname);
>> + printf(_("database cluster.\n\n"));
>
> I doubt this is good line formatting for translation purposes.
>
>> + printf(_("  -c, --check            check data checksums.  This is the default\n"));
>> + printf(_("                         mode if nothing is specified.\n"));
>
> same. For example pg_basebackup uses different multiline style:

Oh, good points.  I forgot about that point of view.

>> +command_fails(['pg_checksums', '--enable', '-r', '1234', '-D', $pgdata],
>> +      "fails when relfilnodes are requested and action is --disable");
>
> action is "--enable" here ;-)

s/relfilnodes/relfilenodes/ while on it.

>> if (badblocks > 0)gi
>> return 1;
>
> Small question: why return 1 instead of exit(1)?

OK, let's fix that on the way as part of the renaming.

>> <refentry id="pgchecksums">
>>  <indexterm zone="pgchecksums">
>
> How about use "app-pgchecksums" similar to other applications?

Yes, I was wondering about that one when doing the renaming, but did
not bother much for consistency.  Anyway switched, you are right.

Attached is an updated patch set, minus the refactoring for
src/common/.
--
Michael

v2-0001-Ensure-version-compatibility-of-pg_verify_checksu.patch (1K) Download Attachment
v2-0002-Rename-pg_verify_checksums-to-pg_checksums.patch (17K) Download Attachment
v2-0003-Add-options-to-enable-and-disable-checksums-in-pg.patch (18K) Download Attachment
v2-0004-Add-option-N-no-sync-to-pg_checksums.patch (6K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
In reply to this post by Michael Banck-2
On Mon, Mar 11, 2019 at 11:19:50AM +0100, Michael Banck wrote:
> One thing: you (Michael) should be co-author for patch #3 as I took some
> of your code from https://github.com/michaelpq/pg_plugins/tree/master/pg
> _checksums

OK, thanks for the notice.  I was not sure as we actually developped
the same fork.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Sergei Kornilov
In reply to this post by Michael Paquier-2
Hi

> Not doing the check is a bad idea as ControlFileData should be compatible
> between the binary and the data read. I am attaching a fresh 0001
> which should be back-patched down to v11 as a bug fix.

Looks fine. We need add few words to documentation?

>>>  if (badblocks > 0)
>>>         return 1;
>>
>>  Small question: why return 1 instead of exit(1)?
>
> OK, let's fix that on the way as part of the renaming.

Was not changed?..

I have no new notes after reading updated patchset.

regards, Sergei

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Banck-2
In reply to this post by Sergei Kornilov
Hi,


Am Montag, den 11.03.2019, 14:11 +0000 schrieb Sergei Kornilov:
> > if (badblocks > 0)
> > return 1;
>
> Small question: why return 1 instead of exit(1)?

I have a feeling it is project policy to return 0 from main(), and
exit(1) if a program aborts with an error.

In the above case, the program finishes more-or-less as intended (no
abort), but due to errors found on the way, does not return with 0.

I don't mind either way and probably exit(1) makes more sense, but I
wanted to explain why it is like that.


Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: [hidden email]

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Sergei Kornilov
Hello

Thank you for explain. I thought so.

PS: I am not sure for now about patch status in CF app. Did not changed status

regards, Sergei

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
In reply to this post by Michael Banck-2
On Tue, Mar 12, 2019 at 11:13:46AM +0100, Michael Banck wrote:
> I have a feeling it is project policy to return 0 from main(), and
> exit(1) if a program aborts with an error.

Yes, it does not matter much in practice, but other tools just don't
do that.  Note that changing it can be actually annoying for a
backpatch if we don't have the --enable/--disable part, because git is
actually smart enough to detect the file renaming across branches as
far as I tried, but as we are refactoring this code anyway for
--enable and --disable let's just do it, that's cleaner.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-4
In reply to this post by Michael Paquier-2

Bonjour Michaël,

Here is a partial review:

> - 0001 if a patch to refactor the routine for the control file
> update.  I have made it backend-aware, and we ought to be careful with
> error handling, use of fds and such, something that v4 was not very
> careful about.

This refactoring patch is ok for me: applies, compiles, check is ok.

However, Am I right in thinking that the change should propagate to other
tools which manipulate the control file, eg pg_resetwal, postmaster… So
that there would be only one shared API to update the control file?

> - 0002 renames pg_verify_checksums to pg_checksums with a
> straight-forward switch.  Docs as well as all references to
> pg_verify_checksums are updated.

Looks ok to me. Applies, compiles, checks are ok. Doc build is ok.

I'm wondering whether there should be something done so that the
inter-release documentation navigation works? Should the section keep the
former name? Is it managed by hand somewhere else? Maybe it would require
to keep the refsect1 id, or to duplicate it, not sure.

In "doc/src/sgml/ref/allfiles.sgml" there seems to be a habit to align on
the SYSTEM keyword, which is not fellowed by the patch.

> - 0003 adds the new options --check, --enable and --disable, with
> --check being the default as discussed.

Looks like the patch I already reviewed, but I'll look at it in details
later.

"If enabling or disabling checksums, the exit status is nonzero if the
operation failed."

However:

  +       if (ControlFile->data_checksum_version == 0 &&
  +               action == PG_ACTION_DISABLE)
  +       {
  +               fprintf(stderr, _("%s: data checksums are already disabled in cluster.\n"), progname);
  +               exit(1);
  +       }

This seem contradictory to me: you want to disable checksum, and they are
already disabled, so nothing is needed. How does that qualifies as a
"failed" operation?

Further review will come later.

> - 0004 adds a -N/--no-sync which I think is nice for consistency with
> other tools.  That's also useful for the tests, and was not discussed
> until now on this thread.

Indeed. I do not immediately see the use case where no syncing would be a
good idea. I can see why it would be a bad idea. So I'm not sure of the
concept.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
On Tue, Mar 12, 2019 at 10:08:19PM +0100, Fabien COELHO wrote:
> This refactoring patch is ok for me: applies, compiles, check is ok.
> However, Am I right in thinking that the change should propagate to other
> tools which manipulate the control file, eg pg_resetwal, postmaster… So that
> there would be only one shared API to update the control file?

Yes, that would be nice, for now I have focused.  For pg_resetwal yes
we could do it easily.  Would you like to send a patch?

> I'm wondering whether there should be something done so that the
> inter-release documentation navigation works? Should the section keep the
> former name? Is it managed by hand somewhere else? Maybe it would require to
> keep the refsect1 id, or to duplicate it, not sure.

When it came to the renaming of pg_receivexlog to pg_receivewal, we
did not actually do anything in the core code, and let the magic
happen on pgsql-www.  I have also pinged pgsql-packagers about the
renaming and it is not really an issue on their side.  So I have
committed the renaming to pg_checksums as well.  So now remains only
the new options.

> In "doc/src/sgml/ref/allfiles.sgml" there seems to be a habit to align on
> the SYSTEM keyword, which is not fellowed by the patch.

Sure.  I sent an updated patch to actually fix that, and also address
a couple of other side things I noticed on the way like the top
refentry in the docs or the header format at the top of
pg_checksums.c as we are on tweaking the area.

> This seem contradictory to me: you want to disable checksum, and they are
> already disabled, so nothing is needed. How does that qualifies as a
> "failed" operation?

If the operation is automated, then a proper reaction can be done if
multiple attempts are done.  Of course, I am fine to tune things one
way or the other depending on the opinion of the crowd here.  From the
opinions gathered, I can see that (Michael * 2) prefer failing with
exit(1), while (Fabien * 1) would like to just do exit(0).

> Further review will come later.

Thanks, Fabien!

> Indeed. I do not immediately see the use case where no syncing would be a
> good idea. I can see why it would be a bad idea. So I'm not sure of the
> concept.

To leverage the buildfarm effort I think this one is worth it.  Or we
finish to fsync the data folder a couple of times, which would make
the small-ish buildfarm machines suffer more than they need.

I am going to send a rebased patch set of the remaining things at the
top of the discussion as well.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
In reply to this post by Michael Paquier-2
On Tue, Mar 12, 2019 at 09:44:03PM +0900, Michael Paquier wrote:
> Yes, it does not matter much in practice, but other tools just don't
> do that.  Note that changing it can be actually annoying for a
> backpatch if we don't have the --enable/--disable part, because git is
> actually smart enough to detect the file renaming across branches as
> far as I tried, but as we are refactoring this code anyway for
> --enable and --disable let's just do it, that's cleaner.

Okay, please find attached a rebased patch set.  I have committed 0001
which adds version checks for the control file, and the renaming
part 0002.  What remains now is the addition of the new options, and
--no-sync.  The "return 1" stuff has been switched to exit(1) while on
it, and is part of 0003.

Now the set of patches is:
- 0001, add --enable and --disable.  I have tweaked a bit the patch so
as "action" is replaced by "mode" which is more consistent with other
tools like pg_ctl.  pg_indent was also complaining about one of the
new enum structures.
- 0002, add --no-sync.

Thanks,
--
Michael

v3-0001-Add-options-to-enable-and-disable-checksums-in-pg.patch (18K) Download Attachment
v3-0002-Add-option-N-no-sync-to-pg_checksums.patch (6K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3
In reply to this post by Michael Paquier-2

Bonjour Michaël,

> Yes, that would be nice, for now I have focused.  For pg_resetwal yes
> we could do it easily.  Would you like to send a patch?

I probably can do that before next Monday. I'll prioritize reviewing the
latest instance of this patch, though.

>> This seem contradictory to me: you want to disable checksum, and they are
>> already disabled, so nothing is needed. How does that qualifies as a
>> "failed" operation?
>
> If the operation is automated, then a proper reaction can be done if
> multiple attempts are done.  Of course, I am fine to tune things one
> way or the other depending on the opinion of the crowd here.  From the
> opinions gathered, I can see that (Michael * 2) prefer failing with
> exit(1), while (Fabien * 1) would like to just do exit(0).

Yep, that sums it up:-).

>> Indeed. I do not immediately see the use case where no syncing would be a
>> good idea. I can see why it would be a bad idea. So I'm not sure of the
>> concept.
>
> To leverage the buildfarm effort I think this one is worth it.  Or we
> finish to fsync the data folder a couple of times, which would make
> the small-ish buildfarm machines suffer more than they need.

Ok for the particular use-case, provided that the documentation is very
clear about the risks, which is the case, so fine with me wrt to the
feature.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
On Wed, Mar 13, 2019 at 07:18:32AM +0100, Fabien COELHO wrote:
> I probably can do that before next Monday. I'll prioritize reviewing the
> latest instance of this patch, though.

Thanks.  The core code of the feature has not really changed with the
last reviews, except for the tweaks in the variable names and I think
that it's in a rather committable shape.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3
In reply to this post by Michael Paquier-2

Michaël-san,

> Now the set of patches is:
> - 0001, add --enable and --disable.  I have tweaked a bit the patch so
> as "action" is replaced by "mode" which is more consistent with other
> tools like pg_ctl.  pg_indent was also complaining about one of the
> new enum structures.

Patch applies cleanly, compiles, various make check ok, doc build ok.

I'm still at odds with the exit(1) behavior when there is nothing to do.

If this behavior is kept, I think that the documentation needs to be
improved because "failed" does not describe a no-op-was-needed to me.

"""
If enabling or disabling checksums, the exit status is nonzero if the
operation failed.
"""

Maybe: "... if the operation failed or the requested setting is already
active." would at least describe clearly the implemented behavior.

  +       printf(_("  -c, --check            check data checksums\n"));
  +       printf(_("                         This is the default mode if nothing is specified.\n"));

I'm not sure of the punctuation logic on the help line: the first sentence
does not end with a ".". I could not find an instance of this style in
other help on pg commands. I'd suggest "check data checksums (default)"
would work around and be more in line with other commands help.

I see a significant locking issue, which I discussed on other threads
without convincing anyone. I could do the following things:

I slowed down pg_checksums by adding a 0.1s sleep when scanning a new
file, then started a "pg_checksums --enable" on a stopped cluster, then
started the cluster while the enabling was in progress, then connected and
updated data. Hmmm. Then I stopped while the slow enabling was still in
progress. Then I could also run a fast pg_checksums --enable in parallel,
overtaking the first one... then when the fast one finished, I started the
cluster again. When the slow one finished, it overwrote the control file,
I had a running cluster with a control file which did not say so, so I
could disable the checksum. Hmmm again. Ok, I could not generate a
inconsistent state because on stopping the cluster the cluster file is
overwritten with the initial state from the point of view of postmater,
but it does not look good.

I do not think it is a good thing that two commands can write to the data
directory at the same time, really.

About fsync-ing: ISTM that it is possible that the control file is written
to disk while data are still not written, so a failure in between would
leave the cluster with an inconsistent state. I think that it should fsync
the data *then* update the control file and fsync again on that one.

> - 0002, add --no-sync.

Patch applies cleanly, compiles, various make checks are ok, doc build ok.

Fine with me.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
On Wed, Mar 13, 2019 at 10:08:33AM +0100, Fabien COELHO wrote:
> I'm not sure of the punctuation logic on the help line: the first sentence
> does not end with a ".". I could not find an instance of this style in other
> help on pg commands. I'd suggest "check data checksums (default)" would work
> around and be more in line with other commands help.

Good idea, let's do that.

> I slowed down pg_checksums by adding a 0.1s sleep when scanning a new file,
> then started a "pg_checksums --enable" on a stopped cluster, then started
> the cluster while the enabling was in progress, then connected and updated
> data.

Well, yes, don't do that.  You can get into the same class of problems
while running pg_rewind, pg_basebackup or even pg_resetwal once the
initial control file check is done for each one of these tools.

> I do not think it is a good thing that two commands can write to the data
> directory at the same time, really.

We don't prevent either a pg_resetwal and a pg_basebackup to run in
parallel.  That would be...  Interesting.

> About fsync-ing: ISTM that it is possible that the control file is written
> to disk while data are still not written, so a failure in between would
> leave the cluster with an inconsistent state. I think that it should fsync
> the data *then* update the control file and fsync again on that one.

If --disable is used, the control file gets updated at the end without
doing anything else.  If the host crashes, it could be possible that
the control file has checksums enabled or disabled.  If the state is
disabled, then well it succeeded.  If the state is enabled, then the
control file is still correct, because all the other blocks still have
checksums set.

if --enable is used, we fsync the whole data directory after writing
all the blocks and updating the control file at the end.  The case you
are referring to here is in fsync_pgdata(), not pg_checksums actually,
because you could reach the same state after a simple initdb.  It
could be possible to reach a state where the control file has
checksums enabled and some blocks are not correctly synced, still you
would notice rather quickly if the server is in an incorrect state at
the follow-up startup.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3

>> I do not think it is a good thing that two commands can write to the data
>> directory at the same time, really.
>
> We don't prevent either a pg_resetwal and a pg_basebackup to run in
> parallel.  That would be...  Interesting.

Yep, I'm trying again to suggest that this kind of thing should be
prevented. It seems that I'm pretty unconvincing.

>> About fsync-ing: ISTM that it is possible that the control file is written
>> to disk while data are still not written, so a failure in between would
>> leave the cluster with an inconsistent state. I think that it should fsync
>> the data *then* update the control file and fsync again on that one.
>
> if --enable is used, we fsync the whole data directory after writing
> all the blocks and updating the control file at the end. [...]
> It could be possible to reach a state where the control file has
> checksums enabled and some blocks are not correctly synced, still you
> would notice rather quickly if the server is in an incorrect state at
> the follow-up startup.

Yep. That is the issue I think is preventable by fsyncing updated data
*then* writing & syncing the control file, and that should be done by
pg_checksums.

--
Fabien.

123456 ... 8