Thread: Yet another small patch - reorderbuffer.c:1099

Hello

There is weired peace of code in reorderbuffer.c:

```
    /* delete from list of known subxacts */
    if (txn->is_known_as_subxact)
    {
        /* NB: nsubxacts count of parent will be too high now */
        dlist_delete(&txn->node);
    }
    /* delete from LSN ordered list of toplevel TXNs */
    else
    {
        dlist_delete(&txn->node);
    }
```

As you see `if` an `else` branches are exactly the same. I wonder
whether this is a bug or code just requires a bit of cleaning. In the
latter case here is a patch.

According to `git log` both branches were introduced in one commit
b89e1510. I added author and committer of this code to CC since they
have much better understanding of it than I do.

--
Best regards,
Aleksander Alekseev
http://eax.me/

On Tue, Apr 5, 2016 at 1:03 AM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> wrote:
> There is weird peace of code in reorderbuffer.c:
>
> ```
>     /* delete from list of known subxacts */
>     if (txn->is_known_as_subxact)
>     {
>         /* NB: nsubxacts count of parent will be too high now */
>         dlist_delete(&txn->node);
>     }
>     /* delete from LSN ordered list of toplevel TXNs */
>     else
>     {
>         dlist_delete(&txn->node);
>     }
> ```
>
> As you see `if` an `else` branches are exactly the same. I wonder
> whether this is a bug or code just requires a bit of cleaning. In the
> latter case here is a patch.
>
> According to `git log` both branches were introduced in one commit
> b89e1510. I added author and committer of this code to CC since they
> have much better understanding of it than I do.

I recall discussing this code with Andres, and I think that he has
mentioned me this is intentional, because should things be changed for
a reason or another in the future, we want to keep in mind that a list
of TXIDs and a list of sub-TXIDs should be handled differently.
-- 
Michael

> I recall discussing this code with Andres, and I think that he has
> mentioned me this is intentional, because should things be changed for
> a reason or another in the future, we want to keep in mind that a list
> of TXIDs and a list of sub-TXIDs should be handled differently.

I see. If this it true I think there should be a comment that explains
it. When you read such a code you suspect a bug. Not mentioning that
static code analyzers (I'm currently experimenting with Clang and PVS
Studio) complain about code like this.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/

On 2016-04-05 12:07:40 +0300, Aleksander Alekseev wrote:
> > I recall discussing this code with Andres, and I think that he has
> > mentioned me this is intentional, because should things be changed for
> > a reason or another in the future, we want to keep in mind that a list
> > of TXIDs and a list of sub-TXIDs should be handled differently.
> 
> I see. If this it true I think there should be a comment that explains
> it. When you read such a code you suspect a bug. Not mentioning that
> static code analyzers (I'm currently experimenting with Clang and PVS
> Studio) complain about code like this.

There's different comments in both branches...

Simon Riggs wrote:
> On 5 April 2016 at 10:12, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2016-04-05 12:07:40 +0300, Aleksander Alekseev wrote:
> > > > I recall discussing this code with Andres, and I think that he has
> > > > mentioned me this is intentional, because should things be changed for
> > > > a reason or another in the future, we want to keep in mind that a list
> > > > of TXIDs and a list of sub-TXIDs should be handled differently.
> > >
> > > I see. If this it true I think there should be a comment that explains
> > > it. When you read such a code you suspect a bug. Not mentioning that
> > > static code analyzers (I'm currently experimenting with Clang and PVS
> > > Studio) complain about code like this.
> >
> > There's different comments in both branches...
> 
> Then one or both of the comments is incomplete.

IMO the code is wrong.  There should be a single block comment saying
something like "Remove the node from its containing list.  In the FOO
case, the list corresponds to BAR and therefore we delete it because
BAZ.  In the QUUX case the list is PLUGH and we delete in because THUD."
Then a single line dlist_delete(...) follows.

The current arrangement looks bizantine to me, for no reason.  If we
think that one of the two branches might do something additional to the
list deletion, surely that will be in a separate stanza with its own
comment; and if we ever want to remove the list deletion from one of the
two cases (something that strikes me as unlikely) then we will need to
fix the comment, too.

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

On Tue, Apr 5, 2016 at 10:38 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> IMO the code is wrong.  There should be a single block comment saying
> something like "Remove the node from its containing list.  In the FOO
> case, the list corresponds to BAR and therefore we delete it because
> BAZ.  In the QUUX case the list is PLUGH and we delete in because THUD."
> Then a single line dlist_delete(...) follows.
>
> The current arrangement looks bizantine to me, for no reason.  If we
> think that one of the two branches might do something additional to the
> list deletion, surely that will be in a separate stanza with its own
> comment; and if we ever want to remove the list deletion from one of the
> two cases (something that strikes me as unlikely) then we will need to
> fix the comment, too.

+1 to everything here except for the way byzantine is spelled.

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

> > IMO the code is wrong.  There should be a single block comment
> > saying something like "Remove the node from its containing list.
> > In the FOO case, the list corresponds to BAR and therefore we
> > delete it because BAZ.  In the QUUX case the list is PLUGH and we
> > delete in because THUD." Then a single line dlist_delete(...)
> > follows.
> >
> > The current arrangement looks bizantine to me, for no reason.  If we
> > think that one of the two branches might do something additional to
> > the list deletion, surely that will be in a separate stanza with
> > its own comment; and if we ever want to remove the list deletion
> > from one of the two cases (something that strikes me as unlikely)
> > then we will need to fix the comment, too.
>
> +1 to everything here except for the way byzantine is spelled.
>

+1 and yet another patch.

--
Best regards,
Aleksander Alekseev
http://eax.me/

On 2016-04-05 11:38:27 -0300, Alvaro Herrera wrote:
> IMO the code is wrong.

I'm a bit confused how an intentionally duplicated block makes code
wrong...

But whatever, I found it to be clerarer that way, but apparently I'm alone.


> The current arrangement looks bizantine to me, for no reason.  If we
> think that one of the two branches might do something additional to the
> list deletion, surely that will be in a separate stanza with its own
> comment; and if we ever want to remove the list deletion from one of the
> two cases (something that strikes me as unlikely) then we will need to
> fix the comment, too.

You realize it's two different lists they're deleted in the different
branches?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2016-04-05 11:38:27 -0300, Alvaro Herrera wrote:
>> The current arrangement looks bizantine to me, for no reason.  If we
>> think that one of the two branches might do something additional to the
>> list deletion, surely that will be in a separate stanza with its own
>> comment; and if we ever want to remove the list deletion from one of the
>> two cases (something that strikes me as unlikely) then we will need to
>> fix the comment, too.

> You realize it's two different lists they're deleted in the different
> branches?

I looked at this and can see some of the argument on both sides, but
if it's setting off static-analyzer warnings for some people, that
seems like a sufficient reason to change it.  We certainly make more
significant changes than this in order to silence warnings.

I rewrote the comment a bit more and pushed it.
        regards, tom lane

> I looked at this and can see some of the argument on both sides, but
> if it's setting off static-analyzer warnings for some people, that
> seems like a sufficient reason to change it.  We certainly make more
> significant changes than this in order to silence warnings.
> 
> I rewrote the comment a bit more and pushed it.

Tom, thank you for committing this patch. And thanks everyone for your
contribution!

-- 
Best regards,
Aleksander Alekseev