Regression tests vs SERIALIZABLE
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)Attachments:
0001-Parallel-regression-tests-don-t-need-workaround-for-.patchtext/x-patch; charset=US-ASCII; name=0001-Parallel-regression-tests-don-t-need-workaround-for-.patchDownload+6-5
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
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.
Attachments:
v2-0001-Parallel-query-regression-tests-don-t-need-SERIAL.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Parallel-query-regression-tests-don-t-need-SERIAL.patchDownload+14-13
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
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!
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.
Attachments:
0001-Fix-transaction.sql-tests-in-higher-isolation-levels.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-transaction.sql-tests-in-higher-isolation-levels.patchDownload+3-1
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
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.
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