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