Re: Offline enabling/disabling of data checksums

Hi,

sorry for letting this slack.

First off, thanks for the review!

Am Mittwoch, den 09.01.2019, 07:07 +0100 schrieb Fabien COELHO:
> > I changed that to the switches -c/--verify (-c for check as -v is taken,
> > should it be --check as well? I personally like verify better), 
> > -d/--disable and -e/--enable.
> 
> I agree that checking the checksum sounds repetitive, but I think that for 
> consistency --check should be provided.

Ok then. The enum is currently called PG_ACTION_VERIFY, I changed that
to PG_ACTION_CHECK as well.

> About the patch: applies, compiles, global & local "make check" are ok.
> 
> There is still no documentation.

I've added that now, though I did that blindly and have not checked the
output yet.

> I think that there is a consensus about renaming the command.

I think so as well, but doing that right now will make the patch
difficult to review, so I'd prefer to leave it to the committer to do
that. 

I can submit a patch with the directory/file rename if that is
preferred. 

> The --help string documents --action which does not exists anymore.

Fixed that.

> The code in "updateControlFile" seems to allow to create the file 
> (O_CREAT). I do not think that it should, it should only apply to an 
> existing file.

Removed that.

> ISTM that some generalized version of this function should be in 
> "src/common/controldata_utils.c" instead of duplicating it from command to 
> command (as suggested by Michaël as well).

Haven't done that yet.

> In "scan_file" verbose output, ISTM that the checksum is more computed 
> than enabled on the file. It is really enabled at the cluster level in the 
> end.

It's certainly not just computed but also written. It's true that it
will be only meaningful if the control file is updated accordingly at
the end, but I don't think that message is very incorrect, so left it
as-is for now.

> Maybe there could be only one open call with a ?: for RO vs RW.

Done that.

> Non root check: as files are only manipulated RW, ISTM that there is no 
> reason why the ownership would be changed, so I do not think that this 
> constraint is useful.

Now that we no longer unlink() pg_control, I believe you are right and I
have removed it.
`
> There is kind of a copy paste for enabling/disabling, I'd consider 
> skipping the scan when not necessary and merge both branches.

Done so.

> > > Also, the full page is rewritten... would it make sense to only overwrite
> > > the checksum part itself?
> > 
> > So just writing the page header? I find that a bit scary and don't
> > expect much speedup as the OS would write the whole block anyway I
> > guess? I haven't touched that yet.
> 
> Possibly the OS would write its block size, which is not necessary the 
> same as postgres page size?

I haven't changed that yet, I think Andres was also of the opinion that
this is not necessary?

> > > It seems that the control file is unlinked and then rewritten. If the
> > > rewritting fails, or the command is interrupted, the user has a problem.
> > > 
> > > Could the control file be simply opened RW?

I've done that now.

New patch attached.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

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