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

Simon Riggs wrote:
> On 5 April 2016 at 10:12, Andres Freund <andres(at)anarazel(dot)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