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 |
Date: | 2019-02-17 18:31:38 |
Message-ID: | 1550428298.12689.12.camel@credativ.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)banck(at)credativ(dot)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
Attachment | Content-Type | Size |
---|---|---|
offline-activation-of-checksums_V3.patch | text/x-patch | 19.6 KB |
From | Date | Subject | |
---|---|---|---|
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 |