Re: [HACKERS] SERIALIZABLE with parallel query

On Mon, Jul 2, 2018 at 3:12 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Jun 29, 2018 at 7:28 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> On Thu, Jun 28, 2018 at 7:55 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> I'd like to test and review this patches but they seem to conflict
>>> with current HEAD. Could you please rebase them?
>>
>> Hi Sawada-san,
>>
>> Thanks! Rebased and attached. The only changes are: the LWLock
>> tranche is now shown as "serializable_xact" instead of "sxact" (hmm,
>> LWLock tranches have lower_case_names_with_underscores, but individual
>> LWLocks have CamelCaseName...), and ShareSerializableXact() no longer
>> does Assert(!IsParallelWorker()). These changes are based on the last
>> feedback from Robert.
>>
>
> Thank you! Will look at patches.
>

I looked at this patches. The latest patch can build without any
errors and warnings and pass all regression tests. I don't see
critical bugs but there are random comments.

+ /*
+ * If the leader in a parallel query earler stashed a partially
+ * released SERIALIZABLEXACT for final clean-up at end
of transaction
+ * (because workers might still have been accessing
it), then it's
+ * time to restore it.
+ */

There is a typo.
s/earler/earlier/

----
Should we add test to check if write-skew[1] anomaly doesn't happen
even in parallel mode?

----
- * We aren't acquiring lightweight locks for the predicate lock or lock
+ * We acquire an LWLock in the case of parallel mode, because worker
+ * backends have access to the leader's SERIALIZABLEXACT. Otherwise,
+ * we aren't acquiring lightweight locks for the predicate lock or lock

There are LWLock and lightweight lock. Maybe it's better to unify the spelling.

----
@@ -2231,9 +2234,12 @@ PrepareTransaction(void)
/*
* Mark serializable transaction as complete for predicate locking
* purposes. This should be done as late as we can put it and
still allow
- * errors to be raised for failure patterns found at commit.
+ * errors to be raised for failure patterns found at commit.
This is not
+ * appropriate for parallel workers however, because we aren't
committing
+ * the leader's transaction and its serializable state will live on.
*/
- PreCommit_CheckForSerializationFailure();
+ if (!IsParallelWorker())
+ PreCommit_CheckForSerializationFailure();

This code assumes that parallel workers could prepare transaction. Is
that expected behavior of parallel query? There is an assertion
!IsInParallelMode() at the beginning of that function though.

----
+ /*
+ * If the transaction is committing, but it has been partially released
+ * already, then treat this as a roll back. It was marked as rolled back.
+ */
+ if (isCommit && SxactIsPartiallyReleased(MySerializableXact))
+ isCommit = false;
+

Isn't it better to add an assertion to check if
MySerializableXact->flags has SXACT_FLAG_ROLLED_BACK flag for safety?

[1] https://en.wikipedia.org/wiki/Snapshot_isolation#Definition

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center