Horton

Thread

Commits

  1. Fix transaction.sql tests in higher isolation levels.

  2. Drop SERIALIZABLE workaround from parallel query tests.

  3. Fix behavior of AND CHAIN outside of explicit transaction blocks

  4. Enable parallel query with SERIALIZABLE isolation.

  1. Regression tests vs SERIALIZABLE

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-15T04:24:08Z

    Hi,

While reviewing the patch for parallel REFRESH MATERIALIZED VIEW, I
noticed that select_parallel.sql and write_parallel.sql believe that
(1) the tests are supposed to work with serializable as a default
isolation level, and (2) parallelism would be inhibited by that, so
they'd better use something else explicitly.  Here's a patch to update
that second thing in light of commit bb16aba5.  I don't think it
matters enough to bother back-patching it.

However, since commit 862ef372d6b, there *is* one test that fails if
you run make installcheck against a cluster running with -c
default_transaction_isolation=serializable: transaction.sql.  Is that
a mistake?  Is it a goal to be able to run this test suite against all
3 isolation levels?

@@ -1032,7 +1032,7 @@
 SHOW transaction_isolation;  -- out of transaction block
  transaction_isolation
 -----------------------
- read committed
+ serializable
 (1 row)

  2. Re: Regression tests vs SERIALIZABLE

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-15T05:14:12Z

    On Mon, Mar 15, 2021 at 9:54 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> While reviewing the patch for parallel REFRESH MATERIALIZED VIEW, I
> noticed that select_parallel.sql and write_parallel.sql believe that
> (1) the tests are supposed to work with serializable as a default
> isolation level, and (2) parallelism would be inhibited by that, so
> they'd better use something else explicitly.  Here's a patch to update
> that second thing in light of commit bb16aba5.  I don't think it
> matters enough to bother back-patching it.

+1, patch basically LGTM. I have one point - do we also need to remove
"begin isolation level repeatable read;" in aggreates.sql, explain.sql
and insert_parallel.sql? And in insert_parallel.sql, the comment also
says "Serializable isolation would disable parallel query", which is
not true after bb16aba5. Do we need to change that too?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  3. Re: Regression tests vs SERIALIZABLE

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-15T05:33:29Z

    On Mon, Mar 15, 2021 at 6:14 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> On Mon, Mar 15, 2021 at 9:54 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > While reviewing the patch for parallel REFRESH MATERIALIZED VIEW, I
> > noticed that select_parallel.sql and write_parallel.sql believe that
> > (1) the tests are supposed to work with serializable as a default
> > isolation level, and (2) parallelism would be inhibited by that, so
> > they'd better use something else explicitly.  Here's a patch to update
> > that second thing in light of commit bb16aba5.  I don't think it
> > matters enough to bother back-patching it.
>
> +1, patch basically LGTM. I have one point - do we also need to remove
> "begin isolation level repeatable read;" in aggreates.sql, explain.sql
> and insert_parallel.sql? And in insert_parallel.sql, the comment also
> says "Serializable isolation would disable parallel query", which is
> not true after bb16aba5. Do we need to change that too?

Yeah, you're right.  That brings us to the attached.

  4. Re: Regression tests vs SERIALIZABLE

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-15T06:59:56Z

    On Mon, Mar 15, 2021 at 11:04 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Mon, Mar 15, 2021 at 6:14 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > On Mon, Mar 15, 2021 at 9:54 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > > While reviewing the patch for parallel REFRESH MATERIALIZED VIEW, I
> > > noticed that select_parallel.sql and write_parallel.sql believe that
> > > (1) the tests are supposed to work with serializable as a default
> > > isolation level, and (2) parallelism would be inhibited by that, so
> > > they'd better use something else explicitly.  Here's a patch to update
> > > that second thing in light of commit bb16aba5.  I don't think it
> > > matters enough to bother back-patching it.
> >
> > +1, patch basically LGTM. I have one point - do we also need to remove
> > "begin isolation level repeatable read;" in aggreates.sql, explain.sql
> > and insert_parallel.sql? And in insert_parallel.sql, the comment also
> > says "Serializable isolation would disable parallel query", which is
> > not true after bb16aba5. Do we need to change that too?
>
> Yeah, you're right.  That brings us to the attached.

Thanks. v2 LGTM, both make check and make check-world passes on my dev system.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  5. Re: Regression tests vs SERIALIZABLE

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-15T10:35:10Z

    On Mon, Mar 15, 2021 at 8:00 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Thanks. v2 LGTM, both make check and make check-world passes on my dev system.

Pushed.  Thanks!

  6. Re: Regression tests vs SERIALIZABLE

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-15T10:51:13Z

    On Mon, Mar 15, 2021 at 5:24 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> However, since commit 862ef372d6b, there *is* one test that fails if
> you run make installcheck against a cluster running with -c
> default_transaction_isolation=serializable: transaction.sql.  Is that
> a mistake?  Is it a goal to be able to run this test suite against all
> 3 isolation levels?

Here's a fix.

  7. Re: Regression tests vs SERIALIZABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-15T14:28:26Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, Mar 15, 2021 at 5:24 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>> However, since commit 862ef372d6b, there *is* one test that fails if
>> you run make installcheck against a cluster running with -c
>> default_transaction_isolation=serializable: transaction.sql.  Is that
>> a mistake?  Is it a goal to be able to run this test suite against all
>> 3 isolation levels?

> Here's a fix.

Usually, if we issue a SET in the regression tests, we explicitly RESET
as soon thereafter as practical, so as to have a well-defined scope
where the script is running under unusual conditions.

			regards, tom lane

  8. Re: Regression tests vs SERIALIZABLE

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-17T04:28:28Z

    On Tue, Mar 16, 2021 at 3:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > On Mon, Mar 15, 2021 at 5:24 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> >> However, since commit 862ef372d6b, there *is* one test that fails if
> >> you run make installcheck against a cluster running with -c
> >> default_transaction_isolation=serializable: transaction.sql.  Is that
> >> a mistake?  Is it a goal to be able to run this test suite against all
> >> 3 isolation levels?
>
> > Here's a fix.
>
> Usually, if we issue a SET in the regression tests, we explicitly RESET
> as soon thereafter as practical, so as to have a well-defined scope
> where the script is running under unusual conditions.

Oh, of course.  Thanks.

I was wrong to blame that commit, and there are many other tests that
fail in the back branches.  But since we were down to just one, I went
ahead and fixed this in the master branch only.

  9. Re: Regression tests vs SERIALIZABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-17T04:31:32Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Tue, Mar 16, 2021 at 3:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Usually, if we issue a SET in the regression tests, we explicitly RESET
>> as soon thereafter as practical, so as to have a well-defined scope
>> where the script is running under unusual conditions.

> Oh, of course.  Thanks.

> I was wrong to blame that commit, and there are many other tests that
> fail in the back branches.  But since we were down to just one, I went
> ahead and fixed this in the master branch only.

Makes sense to me.  Committed patch looks good.

			regards, tom lane