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.
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. I will spend a few more days in testing and review, but figured I should pass along "first impressions" now. On Tue, Jul 10, 2018 at 8:58 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Mon, Jul 2, 2018 at 3:12 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Fri, Jun 29, 2018 at 7:28 PM, Thomas Munro > > <thomas.mu...@enterprisedb.com> wrote: > >> On Thu, Jun 28, 2018 at 7:55 PM, Masahiko Sawada <sawada.m...@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 -- Kevin Grittner VMware vCenter Server https://www.vmware.com/