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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
