Horton

Re: snapbuild woes

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Petr Jelinek <petr.jelinek@2ndquadrant.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>, Erik Rijkers <er@xs4all.nl>, Robert Haas <robertmhaas@gmail.com>, Craig Ringer <craig@2ndquadrant.com>
Date: 2017-05-01T00:59:21Z
Lists: pgsql-hackers 
Hi,

On 2017-04-15 05:18:49 +0200, Petr Jelinek wrote:
> +	/*
> +	 * c) we already seen the xl_running_xacts and tried to do the above.
> +	 * However because of race condition in LogStandbySnapshot() there might
> +	 * have been transaction reported as running but in reality has already
> +	 * written commit record before the xl_running_xacts so decoding has
> +	 * missed it. We now see xl_running_xacts that suggests all transactions
> +	 * from the original one were closed but the consistent state wasn't
> +	 * reached which means the race condition has indeed happened.
> +	 *
> +	 * Start tracking again as if this was the first xl_running_xacts we've
> +	 * seen, with the advantage that because decoding was already running,
> +	 * any transactions committed before the xl_running_xacts record will be
> +	 * known to us so we won't hit with the same issue again.
> +	 */

Unfortunately I don't think that's true, as coded.  You're using
information about committed transactions:

> +	else if (TransactionIdFollows(running->oldestRunningXid,
> +								  builder->running.xmax))
> +	{
> +		int off;
> +
> +		SnapBuildStartXactTracking(builder, running);
> +
>  		/*
> +		 * Nark any transactions that are known to have committed before the
> +		 * xl_running_xacts as finished to avoid the race condition in
> +		 * LogStandbySnapshot().
>  		 *
> +		 * We can use SnapBuildEndTxn directly as it only does the
> +		 * transaction running check and handling without any additional
> +		 * side effects.
>  		 */
> +		for (off = 0; off < builder->committed.xcnt; off++)
> +			SnapBuildEndTxn(builder, lsn, builder->committed.xip[off]);

but a transaction might just have *aborted* before the new snapshot, no?
Since we don't keep track of those, I don't think this guarantees anything?

ISTM, we need a xip_status array in SnapBuild->running.  Then, whenever
a xl_running_xacts is encountered while builder->running.xcnt > 0, loop
for i in 0 .. xcnt_space, and end SnapBuildEndTxn() if xip_status[i] but
the xid is not xl_running_xacts?  Does that make sense?

- Andres

Commits

  1. Fix thinko introduced in 2bef06d516460 et al.

  2. Avoid superfluous work for commits during logical slot creation.

  3. Fix race condition leading to hanging logical slot creation.

  4. Don't use on-disk snapshots for exported logical decoding snapshot.

  5. Preserve required !catalog tuples while computing initial decoding snapshot.