On Thu, Sep 20, 2018 at 9:50 AM Kevin Grittner <kgri...@gmail.com> wrote: > On Tue, Jul 10, 2018 at 8:58 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > 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.
Thanks for the review! And sorry for my delayed response. Here is a rebased patch, with changes as requested. I have replies also for Kevin, see further down. > > + /* > > + * 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/ Fixed. > > ---- > > Should we add test to check if write-skew[1] anomaly doesn't happen > > even in parallel mode? I suppose we could find another one of the existing specs that shows write-skew and adapt it to run a read-only part of the transaction in a parallel worker, but what would it prove that the proposed new test doesn't prove already? > > ---- > > - * 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. Done. > > ---- > > @@ -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. You are right. I made a change exactly like this in CommitTransaction(), where it is necessary, but then somehow I managed to apply that hunk to the identical code in PrepareTransaction() also, where it is not necessary. Fixed. > > ---- > > + /* > > + * 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? That can't hurt. Added. It's don't really the fact that the flag contradicts reality here... but it was already established that the read-only safe optimisation calls ReleasePredicateLocks(false) which behaves like a rollback and marks the SERIALIZABLEXACT that way. I don't have a better idea right now. On Thu, Sep 20, 2018 at 9:50 AM Kevin Grittner <kgri...@gmail.com> wrote: > After reviewing the thread and the current two patches, I agree with > Masahiko Sawada plus preferring one adjustment to the coding: I would > prefer to break out the majority of the ReleasePredicateLocks function > to a static ReleasePredicateLocksMain (or similar) function and > eliminating the goto. Hi Kevin, Thanks for the review. How about moving that bit of local-cleanup code from the end of the function into a new static function ReleasePredicateLocksLocal(), so that we can call it and then return, instead of the evil "goto"? Done that way in the attached. > The optimization in patch 0002 is important. Previous benchmarks > showed a fairly straightforward pgbench test scaled as well as > REPEATABLE READ when it was present, but performance fell off up to > 20% as the scale increased without it. +1 > I will spend a few more days in testing and review, but figured I > should pass along "first impressions" now. Thanks! -- Thomas Munro http://www.enterprisedb.com
0001-Enable-parallel-query-with-SERIALIZABLE-isolatio-v15.patch
Description: Binary data
0002-Enable-the-read-only-SERIALIZABLE-optimization-f-v15.patch
Description: Binary data
