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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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

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(at)gmail(dot)com>wrote:

> On Mon, Jul 1, 2013 at 5:59 PM, Robins Tharakan <tharakan(at)gmail(dot)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(at)gmail(dot)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(dot)janes(at)gmail(dot)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.

--
Robins Tharakan

On Sun, Jul 7, 2013 at 12:49 AM, Robins Tharakan <tharakan(at)gmail(dot)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.
>
>
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

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 11:49 PM, Robins Tharakan <tharakan(at)gmail(dot)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