Hot Standby conflict resolution handling

I was trying some simple queries on a Hot Standby with streaming
replication.

On standby, I do this:
postgres=# begin transaction isolation level repeatable read;
BEGIN
postgres=# explain verbose select count(b) from test WHERE a > 100000;

On master, I insert a bunch of tuples in the table and run VACUUM ANALYZE.
postgres=# INSERT INTO test VALUES (generate_series(110001,120000), 'foo',
1);
INSERT 0 10000
postgres=# VACUUM ANALYZE test;
VACUUM

After max_standby_streaming_delay, the standby starts cancelling the
queries. I get an error like this on the standby:
postgres=# explain verbose select count(b) from test WHERE a > 100000;
FATAL: terminating connection due to conflict with recovery
DETAIL: User query might have needed to see row versions that must be
removed.
HINT: In a moment you should be able to reconnect to the database and
repeat your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.

So I've couple questions/concerns here

1. Why to throw a FATAL error here ? A plain ERROR should be enough to
abort the transaction. There are four places in ProcessInterrupts() where
we throw these kind of errors and three of them are FATAL.

2911 if (DoingCommandRead)
2912 ereport(FATAL,
2913 (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
2914 errmsg("terminating connection due to
conflict with recovery"),
2915 errdetail_recovery_conflict(),
2916 errhint("In a moment you should be able to reconnect
to the"
2917 " database and repeat your command.")));
2918 else
2919 ereport(ERROR,
2920 (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
2921 errmsg("canceling statement due to conflict with
recovery"),
2922 errdetail_recovery_conflict()));

In this particular test case, the backend is DoingCommandRead and that
forces a FATAL error. I'm not sure why is that required. And even if its
necessary, IMHO we should add a comment explaining that. In the other two
places where we throw FATAL, one looks legitimate, but I'm not sure about
the other.

2836 else if (RecoveryConflictPending && RecoveryConflictRetryable)
2837 {
2838 pgstat_report_recovery_conflict(RecoveryConflictReason);
2839 ereport(FATAL,
2840 (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
2841 errmsg("terminating connection due to conflict with
recovery"),
2842 errdetail_recovery_conflict()));
2843 }
2844 else if (RecoveryConflictPending)
2845 {
2846 /* Currently there is only one non-retryable recovery
conflict */
2847 Assert(RecoveryConflictReason ==
PROCSIG_RECOVERY_CONFLICT_DATABASE);
2848 pgstat_report_recovery_conflict(RecoveryConflictReason);
2849 ereport(FATAL,
2850 (errcode(ERRCODE_DATABASE_DROPPED),
2851 errmsg("terminating connection due to conflict with
recovery"),
2852 errdetail_recovery_conflict()));
2853 }

AFAICS the first of these should be ereport(ERROR). Otherwise irrespective
of whether RecoveryConflictRetryable is true or false, we will always
ereport(FATAL).

2. For my test, the error message itself looks wrong because I did not
actually remove any rows on the master. VACUUM probably marked a bunch of
pages as all-visible and that should have triggered a conflict on the
standby in order to support index-only scans. IMHO we should improve the
error message to avoid any confusion. Or we can add a new ProcSignalReason
to differentiate between a cancel due to clean up vs visibilitymap_set()
operation.

BTW, my current set up is using 9.2.1, but I don't see any code changes in
the master. So I would assume the issues will exist there too.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee