On Fri, Apr 22, 2016 at 5:18 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Thu, Apr 21, 2016 at 5:44 AM, Tom Lane <tgl@sss.pgh.pa.us> 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 RecordTransactionAbort@xact.c, 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 RelationTruncate@storage.c 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
