From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Michael Banck <michael(dot)banck(at)credativ(dot)de>, Postgres hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Offline enabling/disabling of data checksums |
Date: | 2019-03-17 11:44:39 |
Message-ID: | alpine.DEB.2.21.1903171119120.2506@lancre |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Michaël-san,
> The issue here is that trying to embed directly the fsync routines
> from file_utils.c into pg_resetwal.c messes up the inclusions because
> pg_resetwal.c includes backend-side includes, which themselves touch
> fd.h :(
>
> In short your approach avoids some extra mess with the include
> dependencies. .
I could remove the two "catalog/" includes from pg_resetwal, I assume that
you meant these ones.
>> Maybe the two changes could be committed separately.
>
> I was thinking about this one, and for pg_rewind we don't care about
> the fsync of the control file because the full data folder gets
> fsync'd afterwards and in the event of a crash in the middle of a
> rewind the target data folder is surely not something to use, but we
> do for pg_checksums, and we do for pg_resetwal. Even if there is the
> argument that usually callers of update_controlfile() would care a
> lot about the control file and fsync it, I think that we need some
> control on if we do the fsync or not because many tools have a
> --no-sync and that should be fully respected.
> So while your patch is on a good track, I would suggest to do the
> following things to complete it:
> - Add an extra argument bits16 to update_controlfile to pass a set of
> optional flags, with NOSYNC being the only and current value. The
> default is to flush the file.
Hmmm. I just did that, but what about just a boolean? What other options
could be required? Maybe some locking/checking?
> - Move the wait event calls WAIT_EVENT_CONTROL_FILE_WRITE_UPDATE and
> WAIT_EVENT_CONTROL_FILE_SYNC_UPDATE to controldata_utils.c.
Done.
> - And then delete UpdateControlFile() in xlog.c, and use
> update_controlfile() instead to remove even more code.
I was keeping that one for another patch because it touches the backend
code, but it makes sense to do that in one go for consistency.
I kept the initial no-parameter function which calls the new one with 4
parameters, though, because it looks more homogeneous this way in the
backend code. This is debatable.
> The version in xlog.c uses BasicOpenFile(), so we should use also that
> in update_controlfile() instead of OpenTransientFile(). As any errors
> result in a PANIC we don't care about leaking fds.
Done.
Attached is an update.
--
Fabien.
Attachment | Content-Type | Size |
---|---|---|
controlfile-update-2.patch | text/x-diff | 8.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dean Rasheed | 2019-03-17 11:47:47 | Re: [HACKERS] PATCH: multivariate histograms and MCV lists |
Previous Message | David Rowley | 2019-03-17 11:40:52 | Re: using index or check in ALTER TABLE SET NOT NULL |