Re: [HACKERS] SERIALIZABLE with parallel query

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