Re: hot_standby_feedback vs excludeVacuum and snapshots

Hi,

On 2018-06-11 10:15:39 +0100, Simon Riggs wrote:
> diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
> index 9db184f8fe..c280744fdd 100644
> --- a/src/backend/storage/ipc/procarray.c
> +++ b/src/backend/storage/ipc/procarray.c
> @@ -1995,10 +1995,6 @@ GetRunningTransactionData(void)
>          volatile PGXACT *pgxact = &allPgXact[pgprocno];
>          TransactionId xid;
>  
> -        /* Ignore procs running LAZY VACUUM */
> -        if (pgxact->vacuumFlags & PROC_IN_VACUUM)
> -            continue;
> -
>          /* Fetch xid just once - see GetNewTransactionId */
>          xid = pgxact->xid;
>  
> @@ -2009,13 +2005,21 @@ GetRunningTransactionData(void)
>          if (!TransactionIdIsValid(xid))
>              continue;
>  
> -        xids[count++] = xid;
> -
> +        /*
> +         * Be careful not to exclude any xids from calculating the values of
> +         * oldestRunningXid and suboverflowed.
> +         */
>          if (TransactionIdPrecedes(xid, oldestRunningXid))
>              oldestRunningXid = xid;
>  
>          if (pgxact->overflowed)
>              suboverflowed = true;
> +
> +        /* Ignore procs running LAZY VACUUM */
> +        if (pgxact->vacuumFlags & PROC_IN_VACUUM)
> +            continue;
> +
> +        xids[count++] = xid;

I don't think this is a good idea. We shouldn't continue down the path
of having running xacts not actually running xacts, but rather go back
to including everything. The case presented in the thread didn't
actually do what it claimed originally, and there's a fair amount of
potential for the excluded xids to cause problems down the line.

Especially not when the fixes should be backpatched.  I think the
earlier patch should be reverted, and then the AEL lock release problem
should be fixed separately.

Greetings,

Andres Freund