Обсуждение: Add regression tests for DISCARD
Hi,
Please find attached a patch that adds basic regression tests for DISCARD command.
Any and all feedback is obviously welcome.
--
Robins Tharakan
Вложения
On Mon, May 13, 2013 at 2:58 AM, Robins Tharakan <tharakan@gmail.com> wrote: > Please find attached a patch that adds basic regression tests for DISCARD > command. > > Any and all feedback is obviously welcome. Perhaps existing tests in guc.sql should be merged into it? -- marko
On 17 June 2013 18:14, Marko Kreen <markokr@gmail.com> wrote:
Perhaps existing tests in guc.sql should be merged into it?
Thanks Marko for pointing out about guc.sql.
Please find attached a patch to move DISCARD related tests from guc.sql to discard.sql. It adds an extra test for a DISCARD PLANS line, although I amn't sure on how to validate that its working.
Personally, I wouldn't call this a great patch, since most of the tests were already running, although in a generic script. The separation of DISCARD related tests to another file is arguably good for the long-term though.
--
Robins Tharakan
Вложения
On Mon, Jul 1, 2013 at 5:59 PM, Robins Tharakan <tharakan@gmail.com> wrote:
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Thanks Marko for pointing out about guc.sql.Please find attached a patch to move DISCARD related tests from guc.sql to discard.sql. It adds an extra test for a DISCARD PLANS line, although I amn't sure on how to validate that its working.Personally, I wouldn't call this a great patch, since most of the tests were already running, although in a generic script. The separation of DISCARD related tests to another file is arguably good for the long-term though.
Robins,
You must add this new test case called "discard" to src/test/regress/parallel_schedule and src/test/regress/serial_schedule, because if we do "make check" the new "discard" test case is not executed.
Regards,
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Thanks Fabrizio.
Although parallel_schedule was a miss for this specific patch, however, I guess I seem to have missed out serial_schedule completely (in all patches) and then thanks for pointing this out. Subsequently Robert too noticed the miss at the serial_schedule end.
Please find attached a patch, updated towards serial_schedule / parallel_schedule as well as the role name change as per Robert's feedback on CREATE OPERATOR thread.
--
Robins Tharakan
On 2 July 2013 09:32, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
On Mon, Jul 1, 2013 at 5:59 PM, Robins Tharakan <tharakan@gmail.com> wrote:Thanks Marko for pointing out about guc.sql.Please find attached a patch to move DISCARD related tests from guc.sql to discard.sql. It adds an extra test for a DISCARD PLANS line, although I amn't sure on how to validate that its working.Personally, I wouldn't call this a great patch, since most of the tests were already running, although in a generic script. The separation of DISCARD related tests to another file is arguably good for the long-term though.Robins,You must add this new test case called "discard" to src/test/regress/parallel_schedule and src/test/regress/serial_schedule, because if we do "make check" the new "discard" test case is not executed.Regards,--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Вложения
On Sat, Jul 6, 2013 at 8:49 PM, Robins Tharakan <tharakan@gmail.com> wrote: > Thanks Fabrizio. > > Although parallel_schedule was a miss for this specific patch, however, I > guess I seem to have missed out serial_schedule completely (in all patches) > and then thanks for pointing this out. Subsequently Robert too noticed the > miss at the serial_schedule end. Why does serial_schedule even exist? Couldn't we just run the parallel schedule serially, like what happens when MAX_CONNECTIONS=1? Cheers, Jeff
On 7 July 2013 14:08, Jeff Janes <jeff.janes@gmail.com> wrote:
Why does serial_schedule even exist? Couldn't we just run the
parallel schedule serially, like what happens when MAX_CONNECTIONS=1?
Well, I am sure it works that way, without errors.
The 'why' still eludes me though, just that its required for this submission.
The 'why' still eludes me though, just that its required for this submission.
--
Robins Tharakan
On Sun, Jul 7, 2013 at 12:49 AM, Robins Tharakan <tharakan@gmail.com> wrote:
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Thanks Fabrizio.Although parallel_schedule was a miss for this specific patch, however, I guess I seem to have missed out serial_schedule completely (in all patches) and then thanks for pointing this out. Subsequently Robert too noticed the miss at the serial_schedule end.Please find attached a patch, updated towards serial_schedule / parallel_schedule as well as the role name change as per Robert's feedback on CREATE OPERATOR thread.
Ok.
Some basic checks on this patch:
- Removed unnecessary extra-lines: Yes
- Cleanly applies to Git-Head: Yes
- Documentation Updated: N/A
- Tests Updated: Yes
- All tests pass: Yes.
- Does it Work: Yes
- Do we want it?: Yes
- Is this a new feature: No
- Does it support pg_dump: No
- Does it follow coding guidelines: Yes
- Any visible issues: No
- Any corner cases missed out: No
- Performance tests required: No
- Any compiler warnings: No
- Are comments sufficient: Yes
- Others: N/A
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
On Sat, Jul 6, 2013 at 11:49 PM, Robins Tharakan <tharakan@gmail.com> wrote: > Thanks Fabrizio. > > Although parallel_schedule was a miss for this specific patch, however, I > guess I seem to have missed out serial_schedule completely (in all patches) > and then thanks for pointing this out. Subsequently Robert too noticed the > miss at the serial_schedule end. > > Please find attached a patch, updated towards serial_schedule / > parallel_schedule as well as the role name change as per Robert's feedback > on CREATE OPERATOR thread. Aside from some reorganization, this patch just checks four new things: - That DISCARD TEMPORARY works like DISCARD TEMP. - That DISCARD PLANS does not throw an error. - That DISCARD ALL fails from within a transaction. - That DISCARD invalid_option fails. The last of these fails with a parse error and therefore, per discussion on the other thread, is not wanted. I'd be more inclined to include the remaining three tests in the existing test file rather than reorganize things into a new file. Reorganizing code makes back-patching harder, and we do back-patch changes that update the regression tests, and I don't think three new tests are are enough justification to add a whole new file. Possibly the test that DISCARD ALL fails from within a transaction ought to be part of a more general category of tests for PreventTransactionChain(). I notice that we currently have NO regression tests that trip that function; we could consider testing some of the others as well. But it's a bit tricky because the CREATE/DROP DATABASE/TABLESPACE commands manipulate global objects - which is a bit hairy - and COMMIT/ROLLBACK PREPARED require special test setup. However, I think we could test CREATE INDEX CONCURRENTLY, DROP INDEX CONCURRENTLY and CLUSTER in addition to DISCARD ALL. Or at least some of those. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company