|From:||Michael Banck <michael(dot)banck(at)credativ(dot)de>|
|To:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||Postgres hackers <pgsql-hackers(at)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz>|
|Subject:||Re: Offline enabling/disabling of data checksums|
|Views:||Raw Message | Whole Thread | Download mbox|
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
> 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
I can submit a patch with the directory/file rename if that is
> The --help string documents --action which does not exists anymore.
> 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.
> 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
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.
> 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.
> > > 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.
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
credativ GmbH, HRB Mönchengladbach 12080
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
|Next Message||Tom Lane||2019-02-17 18:56:23||Re: Actual Cost|
|Previous Message||Tom Lane||2019-02-17 17:40:21||Re: ON SELECT rule on a table without columns|