Better testing coverage and unified coding for plpgsql loops

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Better testing coverage and unified coding for plpgsql loops
Date: 2017-12-30 21:46:41
Message-ID: 26314.1514670401@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

While I've been fooling around with plpgsql, I've been paying close
attention to code coverage reports to make sure that the regression tests
exercise all that new code. It started to bug me that there were some
serious gaps in the test coverage for existing code in pl_exec.c.
One thing I noticed in particular was that coverage for the
PLPGSQL_RC_EXIT/PLPGSQL_RC_RETURN/PLPGSQL_RC_CONTINUE management code
in the various looping statements was nearly nonexistent, and coverage
for integer FOR loops was pretty awful too (eg, not a single BY clause
in the whole test corpus :-(). So I said to myself "let's fix that",
and wrote up a new regression test file plpgsql_control.sql with a
charter to test plpgsql's control structures. I moved a few existing
tests that seemed to fall into that charter into the new file, and
added tests until I was happier with the state of the coverage report.
The result is attached as plpgsql-better-code-coverage.patch.

However, while I was doing that, it seemed like the tests I was adding
were mighty repetitive, as many of them were just exactly the same thing
adjusted for a different kind of loop statement. And so I began to wonder
why it was that we had five copies of the RC_FOO management logic, no two
quite alike. If we only had *one* copy then it would not seem necessary
to have such duplicative test cases for it. A bit of hacking later, and
I had the management logic expressed as a macro, with only one copy for
all five kinds of loop. I verified it still passes the previous set of
tests and then removed the ones that seemed redundant, yielding
plpgsql-unify-loop-rc-code.patch below. So what I propose actually
committing is the combination of these two patches.

Anyone feel like reviewing this, or should I just push it? It seems
pretty noncontroversial to me, but maybe I'm wrong about that.

regards, tom lane

Attachment Content-Type Size
plpgsql-better-code-coverage.patch text/x-diff 55.5 KB
plpgsql-unify-loop-rc-code.patch text/x-diff 22.2 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2017-12-30 21:59:26 Re: pgsql: Add parallel-aware hash joins.
Previous Message Gavin Flower 2017-12-30 21:25:40 Re: What does Time.MAX_VALUE actually represent?