Re: Online enabling of checksums

On 2018-04-06 14:33:48 -0700, Andres Freund wrote:
> On 2018-04-06 02:28:17 +0200, Daniel Gustafsson wrote:
> > Looking into the isolationtester failure on piculet, which builds using
> > --disable-atomics, and locust which doesn’t have atomics, the code for
> > pg_atomic_test_set_flag seems a bit odd.
> >
> > TAS() is defined to return zero if successful, and pg_atomic_test_set_flag()
> > defined to return True if it could set. When running without atomics, don’t we
> > need to do something like the below diff to make these APIs match? :
> >
> > --- a/src/backend/port/atomics.c
> > +++ b/src/backend/port/atomics.c
> > @@ -73,7 +73,7 @@ pg_atomic_init_flag_impl(volatile pg_atomic_flag *ptr)
> > bool
> > pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr)
> > {
> > - return TAS((slock_t *) &ptr->sema);
> > + return TAS((slock_t *) &ptr->sema) == 0;
> > }
>
> Yes, this looks wrong.

And the reason the tests fail reliably after is because the locking
model around ChecksumHelperShmem->launcher_started arguably is broken:

/* If the launcher isn't started, there is nothing to shut down */
if (pg_atomic_unlocked_test_flag(&ChecksumHelperShmem->launcher_started))
return;

This uses a non-concurrency safe primitive. Which then spuriously
triggers:

#define PG_HAVE_ATOMIC_UNLOCKED_TEST_FLAG
static inline bool
pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
{
/*
* Can't do this efficiently in the semaphore based implementation - we'd
* have to try to acquire the semaphore - so always return true. That's
* correct, because this is only an unlocked test anyway. Do this in the
* header so compilers can optimize the test away.
*/
return true;
}

no one can entirely quibble with the rationale that this is ok (I'll
post a patch cleaning up the atomics simulation of flags in a bit), but
this is certainly not a correct locking strategy.

Greetings,

Andres Freund