Review: Fix snapshot taking inconsistencies - Mailing list pgsql-hackers

This is my review on the "Fix snapshot taking inconsistencies patch".

The patch applies against master (a13f12b3a18da0a61571cb134fdecea03a10d6f)


However initdb fails with:

FATAL:  return type mismatch in function declared to return record
DETAIL:  Function's final statement must be SELECT or INSERT/UPDATE/DELETE 
RETURNING.
CONTEXT:  SQL function "ts_debug"

If I run initdb without the patch applied then apply the patch I can test 
the patch.  However the regression tests won't run with the patch applied 
because they call initdb.


Submission:
============
The patch does not include any documentation updates.  I don't feel they
are required.  I can't find anywhere in the documentation where it says
to expect the current behavior.

The patch does not include any regression tests. I don't think we have other 
tests that (can?) test concurrency patterns like this.


Usability review:
=================
The original thread on hackers debated between changing the behavior of 
explain vs changing how rules get executed (so they get a new snapshot). The 
consensus seemed to be to leave explain analyze alone and change the current 
behavior for rules.  This is the route this patch took.

Are there any dangers:  Per Marko's comments on the most recent patch:
"This patch still silently breaks  pg_parse_and_rewrite()..." this still 
seems unresolved.  Marko proposed replacing this with something new for SQL 
functions.  Unfortunately I don't see this as having been followed up on.

I also don't have enough understanding of the code to see exactly how/why it 
was broken or what would be involved in fixing it. I don't know if this is 
why initdb is failing.

With the patch applied SQL functions still can see changes made by other 
transactions after the function starts (ie a function select 
$$ pg_sleep(5);insert into bar select * FROM baz;$$  inserts the row into 
bar both with and without the patch applied.  This is how I would expect a 
function (in SQL or any pl) to work.


Does this patch effect anything outside of rules execution?  Not that I've 
been able to find but I'm probably not qualified to answer that and the 
regression tests don't work.


Performance Review
------------------
I don't see this patch impacting performance

Coding Review
------------------
Coding guidelines: no issues that I can find
Portability: no issues
Windows: not tested but I don't see anything that looks suspicious
Comments: no obvious places that require more comments. I don't feel I have 
a good enough handle on the code to judge the accuracy of the comments.
Does it do what it says: yes
compiler warnings: no
can I make it crash: initdb issue.