Re: [HACKERS] SERIALIZABLE with parallel query

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(dot)mshk(at)gmail(dot)com> wrote:
>
> 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

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/