Re: Online enabling of checksums

On 4/5/18 11:07 AM, Magnus Hagander wrote:
> 
> 
> On Wed, Apr 4, 2018 at 12:11 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote:
>
> ...
>
>     It however still fails to initialize the attempts field after allocating
>     the db entry in BuildDatabaseList, so if you try running with
>     -DRANDOMIZE_ALLOCATED_MEMORY it'll get initialized to values like this:
> 
>      WARNING:  attempts = -1684366952
>      WARNING:  attempts = 1010514489
>      WARNING:  attempts = -1145390664
>      WARNING:  attempts = 1162101570
> 
>     I guess those are not the droids we're looking for?
> 
> 
> When looking at that and after a quick discussion, we just decided it's
> better to completely remove the retry logic. As you mentioned in some
> earlier mail, we had all this logic to retry databases (unlikely) but
> not relations (likely). Attached patch simplifies it to only detect the
> "database was dropped" case (which is fine), and consider every other
> kind of failure a permanent one and just not turn on checksums in those
> cases.
> 

OK, works for me.

> 
> 
>     Likewise, I don't see where ChecksumHelperShmemStruct->abort gets
>     initialized. I think it only ever gets set in launcher_exit(), but that
>     does not seem sufficient. I suspect it's the reason for this behavior:
> 
> 
> It's supposed to get initialized in ChecksumHelperShmemInit() -- fixed.
> (The whole memset-to-zero)
> 

OK, seems fine now.

> 
>         test=# select pg_enable_data_checksums(10, 10);
>         ERROR:  database "template0" does not allow connections
>         HINT:  Allow connections using ALTER DATABASE and try again.
>         test=# alter database template0 allow_connections = true;
>         ALTER DATABASE
>         test=# select pg_enable_data_checksums(10, 10);
>         ERROR:  could not start checksumhelper: already running
>         test=# select pg_disable_data_checksums();
>          pg_disable_data_checksums
>         ---------------------------
> 
>         (1 row)
> 
>         test=# select pg_enable_data_checksums(10, 10);
>         ERROR:  could not start checksumhelper: has been cancelled
> 
> 
> Turns out that wasn't the problem. The problem was that we *set* it
> before erroring out with the "does not allow connections", but never
> cleared it. In that case, it would be listed as launcher-is-running even
> though the launcher was never started.
> 
> Basically the check for datallowconn was put in the wrong place. That
> check should go away completely once we merge (because we should also
> merge the part that allows us to bypass it), but for now I have moved
> the check to the correct place.
> 

Ah, OK. I was just guessing.

> 
> 
>     Attached is a diff with a couple of minor comment tweaks, and correct
>     initialization of the attempts field.
> 
> 
> Thanks. This is included in the attached update, along with the above
> fixes and some other small touches from Daniel. 
> 

This patch version seems fine to me. I'm inclined to mark it RFC.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services