Re: [HACKERS] SERIALIZABLE with parallel query - Mailing list pgsql-hackers

On Thu, Sep 20, 2018 at 9:50 AM Kevin Grittner <kgrittn@gmail.com> wrote:
> On Tue, Jul 10, 2018 at 8:58 PM Masahiko Sawada <sawada.mshk@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 <kgrittn@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