Re: Online enabling of checksums

> On 18 Mar 2018, at 15:02, Michael Banck <michael(dot)banck(at)credativ(dot)de> wrote:
> On Thu, Mar 15, 2018 at 02:01:26PM +0100, Daniel Gustafsson wrote:
>>> On 10 Mar 2018, at 16:09, Michael Banck <michael(dot)banck(at)credativ(dot)de> wrote:
>>> I am aware that this is discussed already, but as-is the user experience
>>> is pretty bad, I think pg_enable_data_checksums() should either bail
>>> earlier if it cannot connect to all databases, or it should be better
>>> documented.
>>
>> Personally I think we should first attempt to solve the "allow-connections in
>> background workers” issue which has been raised on another thread. For now I’m
>> documenting this better.
>
> I had a look at that thread and it seems stalled, I am a bit worried
> that this will not be solved before the end of the CF.
>
> So I think unless the above gets solved, pg_enable_data_checksums()
> should error out with the hint. I've had a quick look and it seems one
> can partly duplicate the check from BuildDatabaseList() (or optionally
> move it) to the beginning of StartChecksumHelperLauncher(), see
> attached.

I’ve incorporated a slightly massaged version of your patch. While it is a
little ugly to duplicate the logic, it’s hopefully a short-term fix, and I
agree that silently failing is even uglier. Thanks for the proposal!

It should be noted that a database may well be altered to not allow connections
between the checksum helper starts, or gets the database list, and when it
tries to checksum it, so we might still fail on this very issue with the checks
bypassed. Still improves the UX to catch low hanging fruit of course.

> The following errmsg() capitalize the error message without the first
> word being a specific term, which I believe is not project style:

Fixed.

> Also, in src/backend/postmaster/checksumhelper.c there are few
> multi-line comments (which are not function comments) that do not have a
> full stop at the end, which I think is also project style:

I’ve addressed all of these, but I did leave one as a multi-line which I think
looks better as that. I also did some spellchecking and general tidying up of
the error messages and comments in the checksum helper.

> Apart from that, I previously complained about the error in
> pg_verify_checksums:

Updated to your suggestion. While in there I re-wrote a few other error
messages to be consistent in message and quoting.

> Otherwise, I had a quick look over v4 and found no further issues.
> Hopefully I will be able to test it on some bigger test databases next
> week.

Thanks a lot for your reviews!

cheers ./daniel