Re: [HACKERS] SERIALIZABLE with parallel query

On Mon, Jul 2, 2018 at 3:12 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Fri, Jun 29, 2018 at 7:28 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> On Thu, Jun 28, 2018 at 7:55 PM, Masahiko Sawada <sawada.mshk@gmail.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