On Fri, Apr 22, 2016 at 5:18 AM, Alvaro Herrera <[email protected]> wrote: > Michael Paquier wrote: >> On Thu, Apr 21, 2016 at 5:44 AM, Tom Lane <[email protected]> wrote: >> > Anyway, I went through our tree and added START/END_CRIT_SECTION calls >> > around all XLogInsert calls that could currently be reached without one; >> > see attached. Since this potentially breaks third-party code I would >> > not propose back-patching it, but I think it's reasonable to propose >> > applying it to HEAD. >> >> +1 for sanitizing those code paths this way. This patch looks sane to >> me after having a look with some testing. >> >> --- a/src/backend/access/brin/brin.c >> +++ b/src/backend/access/brin/brin.c >> @@ -610,15 +610,12 @@ brinbuild(Relation heap, Relation index, >> IndexInfo *indexInfo) >> elog(ERROR, "index \"%s\" already contains data", >> RelationGetRelationName(index)); >> >> - /* >> - * Critical section not required, because on error the creation of the >> - * whole relation will be rolled back. >> - */ >> Perhaps Alvaro has a opinion to offer regarding this bit removed in brin.c? > > I vaguely recall copying this comment from elsewhere, but I didn't see > any other such comment being removed by the patch; I probably copied > something else which got slowly mutated into what's there today during > development. > > if we're adding the critical section then the comment should > certainly be removed too.
A scan of the code is showing me that there are 88 sections in the code containing a comment referring to a critical section, actually a little bit more because those two terms are sometimes broken between two lines. With Tom's patch applied, I have found two inconsistencies. In [email protected], there is the following comment that would need a refresh because XactLogAbortRecord logs a record: /* XXX do we really need a critical section here? */ START_CRIT_SECTION(); The comment of [email protected] referring to the use of critical sections should be updated. Looking at that the access code while going through the patch, perhaps it would be good to add some assertions regarding the presence of a critical section in some code paths of gin. For example dataExecPlaceToPage and entryExecPlaceToPage should be invoked in a critical section per their comments. heap_page_prune_execute, spgPageIndexMultiDelete, spgFormDeadTuple could be as well candidates for such changes. Surely that's a different, HEAD-only patch though. -- Michael -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
