Date: Sun, 3 Apr 2022 22:52:18 -0700
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@lists.postgresql.org
Subject: Re: Race condition in server-crash testing
Message-ID: <20220404055218.GD3799006@rfd.leadboat.com>
References: <1801850.1649047827@sss.pgh.pa.us>
 <20220404050721.sktsuearncwjo6hr@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220404050721.sktsuearncwjo6hr@alap3.anarazel.de>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: <https://www.postgresql.org/message-id/20220404055218.GD3799006%40rfd.leadboat.com>
Precedence: bulk

On Sun, Apr 03, 2022 at 10:07:21PM -0700, Andres Freund wrote:
> On 2022-04-04 00:50:27 -0400, Tom Lane wrote:
> > My pet dinosaur gaur just failed [1] in
> > src/test/recovery/t/022_crash_temp_files.pl, which does this:
> > 
> > -----
> > my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid);
> > is($ret, 0, 'killed process with KILL');
> > 
> > # Close psql session
> > $killme->finish;
> > $killme2->finish;
> > 
> > # Wait till server restarts
> > $node->poll_query_until('postgres', undef, '');
> > -----
> > 
> > It's hard to be totally sure, but I think what happened is that
> > gaur hit the in-hindsight-obvious race condition in this code:
> > we managed to execute a successful iteration of poll_query_until
> > before the postmaster had noticed its dead child and commenced
> > the restart.  The test lines after these are not prepared to see
> > failure-to-connect.
> > 
> > It's not obvious to me how to remove this race condition.
> > Thoughts?
> 
> Maybe we can use pump_until() with the psql that's not getting killed? With a
> non-matching regex? That'd only return once the backend was killed by
> postmaster, afaics?

Sounds good; I suspect that will be better than any of the ideas I scratched
down when
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2021-08-31%2015%3A00%3A52
failed the same way.  For what it's worth, those were:

- Check that pg_postmaster_start_time() has changed.  But that runs into EPIPE
  trouble, requiring a write to a file or an eval{} to trap the EPIPE.
- Likewise, but check "select checkpoint_lsn from pg_control_checkpoint();".
- Poll pg_controldata until a new checkpoint happens.  Compare checkpoint LSN.
  Use checkpoint_timeout=1h to avoid non-end-of-recovery checkpoints.
- Poll logfile until "all server processes terminated; reinitializing".  Can
  be fooled with certain log_min_messages settings, but so can our other
  log-scraping tests.
- Grab the pid of e.g. the checkpointer and poll for that process to be gone.
  Can be fooled by PID reuse.