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.mshk@gmail.com> wrote:
>
> 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



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