Better testing coverage and unified coding for plpgsql loops

2017-12-30 22:46 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

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.

I don't think any issue there. This macro is little bit massive, but
similar is used elsewhere.

+1

Pavel

Tom Lane wrote:

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'm not entirely sure about this rationale. Improving coverage is of
course an important goal, but it's not the only one: each documented
construct and behavior should be tested also explicitly, to avoid any
inadvertant breakage. You're probably the most careful committer in the
project, but what that means is that some other less careful committer
(present or future) will need to hack this code again in the future and
break some of the cases that you've made to work, because the test cases
only exercise the generic behavior through some specific kind of loop,
and not every individual kind of loop specifically.

In other words, I think testing (the basic behavior of) every construct
separately is worthwhile even if it tests the same code several times.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello!

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.

I have looked into plpgsql-unify-loop-rc-code.patch.
I have two questions:

- how do currently existing coverage tools display coverage for such a
large macro?

I expect DEFINE's to be treated as comments.

I've looked into https://coverage.postgresql.org/src/port/qsort.c.gcov.html and
on line 70 I see a similar multi line define that is yellow in coverage,
not counted at all. I think that "higher coverage" effect you are seeing is
mostly due to code being hidden from coverage counter, not actually better
testing. Another thing I see is that most define's are in .h files, and
they're also not in coverage report mostly.

- can this macro become a function?

Darafei "Komяpa" Praliaskouski wrote:

- how do currently existing coverage tools display coverage for such a
large macro?

I expect DEFINE's to be treated as comments.

It is, but then it is counted in the callsite where each branch is
displayed separately. So in
https://coverage.postgresql.org/src/pl/plpgsql/src/pl_exec.c.gcov.html
line 2028 you can see a bunch of "+" and three "-".

- can this macro become a function?

The "exit_action" argument makes it tough. It can probably be done --
it seems to require contorting the one callsite that uses "goto" though.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Darafei "Komяpa" Praliaskouski wrote:

- can this macro become a function?

The "exit_action" argument makes it tough. It can probably be done --
it seems to require contorting the one callsite that uses "goto" though.

It could be converted into a function returning bool, a la

if (!loop_rc_processing(...))
break;

but then the burden is on you to show there's negligible performance
impact, a question that doesn't arise when just macro-izing existing
code. I suppose the function could be made inline, but then we're
right back to the question of how well lcov will display the actual
code coverage.

regards, tom lane

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Tom Lane wrote:

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'm not entirely sure about this rationale. Improving coverage is of
course an important goal, but it's not the only one: each documented
construct and behavior should be tested also explicitly, to avoid any
inadvertant breakage. You're probably the most careful committer in the
project, but what that means is that some other less careful committer
(present or future) will need to hack this code again in the future and
break some of the cases that you've made to work, because the test cases
only exercise the generic behavior through some specific kind of loop,
and not every individual kind of loop specifically.

I don't especially buy this argument, at least not in this case.
I can certainly believe that somebody would add another looping construct
to plpgsql in future, but with the new setup they'd just copy-and-paste
a macro invocation, and there's basically nothing to break. Fooling
with the RC logic itself is so rare as to be negligible --- looking at
the git history, most of it sprang full grown from Jan Wieck's forehead
in 1998, and the only meaningful change since then was when Neil Conway
added CONTINUE in 2005. So I think you're proposing to add regression
testing overhead that will burden every developer, every day, for the
foreseeable future, for a really negligible return. That path leads to
regression tests that nobody runs because they take two hours.

regards, tom lane

On Tue, Jan 2, 2018 at 10:08 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Darafei "Komяpa" Praliaskouski wrote:

- can this macro become a function?

The "exit_action" argument makes it tough. It can probably be done --
it seems to require contorting the one callsite that uses "goto" though.

It could be converted into a function returning bool, a la

if (!loop_rc_processing(...))
break;

but then the burden is on you to show there's negligible performance
impact, a question that doesn't arise when just macro-izing existing
code. I suppose the function could be made inline, but then we're
right back to the question of how well lcov will display the actual
code coverage.

I prefer writing this sort of thing using a function call and
dispatching on the return value, as you suggest in the text quoted
here. Long macros that involve a zillion continuation lines are hard
to edit, and often hard to step through properly in a debugger. It
may be that in this case it doesn't matter much because, as you said
in the other email, this code may have no bugs and never get modified;
if so, we don't care whether it's hard to edit and debug the code. Of
course, then we also don't really need tests for it, either.

I disagree that Alvaro or Darafei must carry the burden of proving
that using a function rather than a macro doesn't slow anything down.
In general, it's not like performance concerns unilaterally trump
readability. But in this case, I suspect it would be hard to prove
anything at all. It seems unlikely to be in the first place that any
change would be anything more than noise, and there's the sort of
broader issues that duplicating the code makes the binary bigger,
which carries a distributed cost of its own. I'm not sure how you'd
even design a fair test for something like this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Jan 2, 2018 at 10:08 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

It could be converted into a function returning bool, a la
if (!loop_rc_processing(...))
break;

I prefer writing this sort of thing using a function call and
dispatching on the return value, as you suggest in the text quoted
here. Long macros that involve a zillion continuation lines are hard
to edit, and often hard to step through properly in a debugger.

I thought about this a bit harder and realized that if we make it
a function, we will have to pass "rc" by reference since the function
needs to change it in some cases. That might have no impact if the
compiler is smart enough, but I expect on at least some compilers
it would end up forcing rc into memory with an attendant speed hit.

I really think we should stick with the macro implementation, unless
somebody wants to do some actual investigation to prove that a
function implementation imposes negligible cost. I'm not prepared
to just assume that, especially not after the work I just did on
plpgsql record processing --- I initially thought that an extra
function call or three wouldn't matter in those code paths either,
but I found out differently.

regards, tom lane

On Wed, Jan 3, 2018 at 1:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I thought about this a bit harder and realized that if we make it
a function, we will have to pass "rc" by reference since the function
needs to change it in some cases. That might have no impact if the
compiler is smart enough, but I expect on at least some compilers
it would end up forcing rc into memory with an attendant speed hit.

I really think we should stick with the macro implementation, unless
somebody wants to do some actual investigation to prove that a
function implementation imposes negligible cost. I'm not prepared
to just assume that, especially not after the work I just did on
plpgsql record processing --- I initially thought that an extra
function call or three wouldn't matter in those code paths either,
but I found out differently.

OK. I'm not really exercised about it, so I'll leave it to others to
decide whether they want to spend time on it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tom Lane wrote:

I really think we should stick with the macro implementation, unless
somebody wants to do some actual investigation to prove that a
function implementation imposes negligible cost. I'm not prepared
to just assume that, especially not after the work I just did on
plpgsql record processing --- I initially thought that an extra
function call or three wouldn't matter in those code paths either,
but I found out differently.

I don't really care too much about the macro-or-function side of this,
but if you wanted to improve debuggability avoiding the performance cost
of a function call, you could use a static inline function, which is
supposed (AFAIK) to have performance characteristics equivalent to those
of a macro. But again I'm not voting either way and I'm not in a
position to do the legwork either.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Tom Lane wrote:

I really think we should stick with the macro implementation, unless
somebody wants to do some actual investigation to prove that a
function implementation imposes negligible cost.

I don't really care too much about the macro-or-function side of this,
but if you wanted to improve debuggability avoiding the performance cost
of a function call, you could use a static inline function, which is
supposed (AFAIK) to have performance characteristics equivalent to those
of a macro.

I'm not sure whether inlining the function can be relied on to get rid
of the side effects of taking rc's address. It wouldn't take all that
much work to establish the point, probably, but it's work I don't care
to put into it.

regards, tom lane