Thread: Shortening the Scope of Critical Section in Creation of New MultiXacts

Hi all, 

I have been going through the multixact code over the last few 
weeks, and noticed a potential discrepancy between the need for critical 
sections in the creation of new multixacts and the current code.

As per the comment in GetNewMultiXact:


    /*
     * Critical section from here until caller has written the data into the
     * just-reserved SLRU space; we don't want to error out with a partly
     * written MultiXact structure.  (In particular, failing to write our
     * start offset after advancing nextMXact would effectively corrupt the
     * previous MultiXact.)
     */
    START_CRIT_SECTION()


This makes sense, as we need the multixact state and multixact offset 
data to be consistent, but once we write data into the offsets, we can 
end the critical section. Currently we wait until the members data is 
also written before we end the critical section. 


I’ve attached a patch that moves the end of the critical section into 
RecordNewMultiXact, right after we finish writing data into the offsets 
cache.

This passes regression tests, as well as some custom testing around 
injecting random failures while writing multixact members, and 
restarting.

I would appreciate any feedback on this.


Sincerely, 

Rishu Bagga, Amazon Web Services (AWS)

Hello.

In short, the code as-is looks correct.

At Wed, 14 Dec 2022 00:14:34 +0000, "Bagga, Rishu" <bagrishu@amazon.com> wrote in 
>     * Critical section from here until caller has written the data into the
>     * just-reserved SLRU space; we don't want to error out with a partly

"the data" here means the whole this multi transaction, which includes
members. We shouldn't exit the critical section at least until the
very end of RecordNewMultiXact().

> This makes sense, as we need the multixact state and multixact offset 
> data to be consistent, but once we write data into the offsets, we can 
> end the critical section. Currently we wait until the members data is 
> also written before we end the critical section. 

Why do you think that the members are not a part of a
multitransaction?  A multitransaction is not complete without them.

Addition to the above, we cannot simply move the END_CRIT_SECTION() to
there since RecordNewMultiXact() has another caller that doesn't start
a critical section.

By the way, I didn't tested this for myself but..

> This passes regression tests

Maybe if you did the same with an assertion-build, you will get an
assertion-failure.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center