Обсуждение: "Some tests to cover hash_index"
I am attaching the patch to improve some coverage of hash index code [1].
I have added some basic tests, which mainly covers overflow pages. It took 2 sec extra time in my machine in parallel schedule.
[1]Concurrent Hash Index.
--
I have added some basic tests, which mainly covers overflow pages. It took 2 sec extra time in my machine in parallel schedule.
| Hit | Total | Coverage | ||
| old tests | Line Coverage | 780 | 1478 | 52.7 |
| Function Coverage | 63 | 85 | 74.1 | |
| improvement after tests | Line Coverage | 1181 | 1478 | 79.9 % |
| Function Coverage | 78 | 85 | 91.8 % |
[1]Concurrent Hash Index.
Вложения
On Thu, Aug 4, 2016 at 7:24 PM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:
+--
+-- Cause some overflow insert and splits.
+--
+CREATE TABLE con_hash_index_table (keycol INT);
+CREATE INDEX con_hash_index on con_hash_index_table USING HASH (keycol);
I am attaching the patch to improve some coverage of hash index code [1].
I have added some basic tests, which mainly covers overflow pages. It took 2 sec extra time in my machine in parallel schedule.
Hit Total Coverage old tests Line Coverage 780 1478 52.7 Function Coverage 63 85 74.1 improvement after tests Line Coverage 1181 1478 79.9 % Function Coverage 78 85 91.8 %
I think the code coverage improvement for hash index with these tests seems to be quite good, however time for tests seems to be slightly on higher side. Do anybody have better suggestion for these tests?
diff --git a/src/test/regress/sql/concurrent_hash_index.sql b/src/test/regress/sql/concurrent_hash_index.sql
I wonder why you have included a new file for these tests, why can't be these added to existing hash_index.sql.
+-- Cause some overflow insert and splits.
+--
+CREATE TABLE con_hash_index_table (keycol INT);
+CREATE INDEX con_hash_index on con_hash_index_table USING HASH (keycol);
The relation name con_hash_index* choosen in above tests doesn't seem to be appropriate, how about hash_split_heap* or something like that.
Register your patch in latest CF (https://commitfest.postgresql.org/10/)
Hi All,
I have reverified the code coverage for hash index code using the test file (commit-hash_coverage_test) attached with this mailing list and have found that some of the code in _hash_squeezebucket() function flow is not being covered. For this i have added a small testcase on top of 'commit hash_coverage_test' patch. I have done this mainly to test Amit's WAL for hash index patch [1].
I have also removed the warning message that we used to get for hash index like 'WARNING: hash indexes are not WAL-logged and their use is discouraged' as this message is now no more visible w.r.t hash index after the WAL patch for hash index. Please have a look and let me know your thoughts.
[1] - https://www.postgresql.org/message-id/CAA4eK1JOBX%3DYU33631Qh-XivYXtPSALh514%2BjR8XeD7v%2BK3r_Q%40mail.gmail.com
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
I have reverified the code coverage for hash index code using the test file (commit-hash_coverage_test) attached with this mailing list and have found that some of the code in _hash_squeezebucket() function flow is not being covered. For this i have added a small testcase on top of 'commit hash_coverage_test' patch. I have done this mainly to test Amit's WAL for hash index patch [1].
I have also removed the warning message that we used to get for hash index like 'WARNING: hash indexes are not WAL-logged and their use is discouraged' as this message is now no more visible w.r.t hash index after the WAL patch for hash index. Please have a look and let me know your thoughts.
[1] - https://www.postgresql.org/message-id/CAA4eK1JOBX%3DYU33631Qh-XivYXtPSALh514%2BjR8XeD7v%2BK3r_Q%40mail.gmail.com
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
On Sat, Aug 6, 2016 at 9:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Aug 4, 2016 at 7:24 PM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:I am attaching the patch to improve some coverage of hash index code [1].
I have added some basic tests, which mainly covers overflow pages. It took 2 sec extra time in my machine in parallel schedule.
Hit Total Coverage old tests Line Coverage 780 1478 52.7 Function Coverage 63 85 74.1 improvement after tests Line Coverage 1181 1478 79.9 % Function Coverage 78 85 91.8 % I think the code coverage improvement for hash index with these tests seems to be quite good, however time for tests seems to be slightly on higher side. Do anybody have better suggestion for these tests?diff --git a/src/test/regress/sql/
concurrent_hash_index.sql b/src/test/regress/sql/ concurrent_hash_index.sql I wonder why you have included a new file for these tests, why can't be these added to existing hash_index.sql.+--
+-- Cause some overflow insert and splits.
+--
+CREATE TABLE con_hash_index_table (keycol INT);
+CREATE INDEX con_hash_index on con_hash_index_table USING HASH (keycol);The relation name con_hash_index* choosen in above tests doesn't seem to be appropriate, how about hash_split_heap* or something like that.Register your patch in latest CF (https://commitfest.postgresql.org/10/) --
Hi,
I missed to attach the patch in my previous mail. Here i attach the patch.
On Tue, Aug 23, 2016 at 11:47 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
Hi All,
I have reverified the code coverage for hash index code using the test file (commit-hash_coverage_test) attached with this mailing list and have found that some of the code in _hash_squeezebucket() function flow is not being covered. For this i have added a small testcase on top of 'commit hash_coverage_test' patch. I have done this mainly to test Amit's WAL for hash index patch [1].
I have also removed the warning message that we used to get for hash index like 'WARNING: hash indexes are not WAL-logged and their use is discouraged' as this message is now no more visible w.r.t hash index after the WAL patch for hash index. Please have a look and let me know your thoughts.
[1] - https://www.postgresql.org/message-id/CAA4eK1JOBX% 3DYU33631Qh-XivYXtPSALh514% 2BjR8XeD7v%2BK3r_Q%40mail. gmail.com
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com On Sat, Aug 6, 2016 at 9:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:On Thu, Aug 4, 2016 at 7:24 PM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:I am attaching the patch to improve some coverage of hash index code [1].
I have added some basic tests, which mainly covers overflow pages. It took 2 sec extra time in my machine in parallel schedule.
Hit Total Coverage old tests Line Coverage 780 1478 52.7 Function Coverage 63 85 74.1 improvement after tests Line Coverage 1181 1478 79.9 % Function Coverage 78 85 91.8 % I think the code coverage improvement for hash index with these tests seems to be quite good, however time for tests seems to be slightly on higher side. Do anybody have better suggestion for these tests?diff --git a/src/test/regress/sql/concurr
ent_hash_index.sql b/src/test/regress/sql/concurr ent_hash_index.sql I wonder why you have included a new file for these tests, why can't be these added to existing hash_index.sql.+--
+-- Cause some overflow insert and splits.
+--
+CREATE TABLE con_hash_index_table (keycol INT);
+CREATE INDEX con_hash_index on con_hash_index_table USING HASH (keycol);The relation name con_hash_index* choosen in above tests doesn't seem to be appropriate, how about hash_split_heap* or something like that.Register your patch in latest CF (https://commitfest.postgresql.org/10/) --
Вложения
On Tue, Aug 23, 2016 at 2:17 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > I have also removed the warning message that we used to get for hash index like 'WARNING: hash indexes are not WAL-loggedand their use is discouraged' as this message is now no more visible w.r.t hash index after the WAL patch for hashindex. Please have a look and let me know your thoughts. Well, that change should be part of Amit's patch to add WAL logging, not this patch, whose mission is just to improve test coverage. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, > Well, that change should be part of Amit's patch to add WAL logging, > not this patch, whose mission is just to improve test coverage. I have just removed the warning message from expected output file as i have performed the testing on Amit's latest patch that removes this warning message from the hash index code.
On Wed, Aug 24, 2016 at 11:38 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > Hi, > >> Well, that change should be part of Amit's patch to add WAL logging, >> not this patch, whose mission is just to improve test coverage. > > I have just removed the warning message from expected output file as i > have performed the testing on Amit's latest patch that removes this > warning message from the hash index code. > I think you are missing what Robert wants to point out. You don't need to remove the warning message when you are adding new test cases on top of Mithun's patch even if those new tests helps to cover the code path which is required for wal-hash-index patch. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Aug 24, 2016 at 2:34 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Aug 24, 2016 at 11:38 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >>> Well, that change should be part of Amit's patch to add WAL logging, >>> not this patch, whose mission is just to improve test coverage. >> >> I have just removed the warning message from expected output file as i >> have performed the testing on Amit's latest patch that removes this >> warning message from the hash index code. >> > > I think you are missing what Robert wants to point out. You don't > need to remove the warning message when you are adding new test cases > on top of Mithun's patch even if those new tests helps to cover the > code path which is required for wal-hash-index patch. Right. The point is, if somebody applies this patch on top of master, the regression tests will now fail because of that missing line. That means nobody is going to commit this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Aug 6, 2016 at 9:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I wonder why you have included a new file for these tests, why can't be these added to existing hash_index.sql.
Fixed. Have renamed relation, index and test filename accordingly.tests in hash_index.sql did not cover overflow pages, above tests were for mainly for them. So I thought having a separate test file can help enabling/disabling them in schedule files, when we do not want them running as it take slightly high time. If you think otherwise I will reconsider will add tests to hash_index.sql.
>The relation name con_hash_index* choosen in above tests doesn't seem to be appropriate, how about hash_split_heap* or something like that.
Вложения
On Mon, Sep 19, 2016 at 8:44 PM, Mithun Cy <mithun.cy@enterprisedb.com> wrote: > On Sat, Aug 6, 2016 at 9:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> I wonder why you have included a new file for these tests, why can't be >> these added to existing hash_index.sql. > tests in hash_index.sql did not cover overflow pages, above tests were for > mainly for them. > The name of file hash_index_split suggests it focus on split. Are the tests focussed more on overflow pages or on split of hash index? So I thought having a separate test file can help > enabling/disabling them in schedule files, when we do not want them running > as it take slightly high time. If you think otherwise I will reconsider will > add tests to hash_index.sql. > I think you have a point, but not sure if it is worth to add a separate file. It might be tricky to choose which file to add new tests for hash_indexes. Anybody else have opinion on this point? Can you check how much time it takes as compare to btree or brin index tests? I am facing below diff with your new patch. *************** *** 1,4 **** ! -- -- Cause some overflow insert and splits. -- CREATE TABLE hash_split_heap (keycol INT); --- 1,4 ---- ! -- -- Cause some overflow insert and splits. -- CREATE TABLE hash_split_heap (keycol INT); ====================================================================== There is an extra space in expected file which is leading to above failure. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Sep 20, 2016 at 8:52 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > I think you have a point, but not sure if it is worth to add a > separate file. It might be tricky to choose which file to add new > tests for hash_indexes. Anybody else have opinion on this point? I think all the tests should be added to hash_index.sql. A new file doesn't seem warranted. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Why not use generate_series() queries to insert the appropriate number of tuples, instead of a handful of INSERT lines each time? Since each insert is a separate transaction, that would probably be faster. Why do you have a plpgsql function just to create a cursor? Wouldn't it be simpler to create the cursor in an SQL statement? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Sep 20, 2016 at 2:26 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Why not use generate_series() queries to insert the appropriate number > of tuples, instead of a handful of INSERT lines each time? Since each > insert is a separate transaction, that would probably be faster. > > Why do you have a plpgsql function just to create a cursor? Wouldn't it > be simpler to create the cursor in an SQL statement? This patch hasn't been updated in over a week, so I'm marking it Returned with Feedback. I think this is a good effort and I hope something committable will come from it, but with 2 days left it's not going to happen this CF. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Sep 28, 2016 at 9:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> something committable will come from it, but with 2 days left it's not
> going to happen this CF.
Adding a new patch. This one uses generate series instead of INSERT INTO SELECT and fixed comments from Alvaro. Вложения
On Wed, Oct 12, 2016 at 1:17 AM, Mithun Cy <mithun.cy@enterprisedb.com> wrote: > On Wed, Sep 28, 2016 at 9:56 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> something committable will come from it, but with 2 days left it's not >> going to happen this CF. > Adding a new patch. This one uses generate series instead of INSERT INTO > SELECT and fixed comments from Alvaro. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company