Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Aleksander Alekseev]

> 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Tom Lane]

Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Aleksander Alekseev]

> > 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/
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 52c6986..360e6fd 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1159,17 +1159,17 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 		txn->base_snapshot_lsn = InvalidXLogRecPtr;
 	}
 
-	/* delete from list of known subxacts */
-	if (txn->is_known_as_subxact)
-	{
-		/* NB: nsubxacts count of parent will be too high now */
-		dlist_delete(>node);
-	}
-	/* delete from LSN ordered list of toplevel TXNs */
-	else
-	{
-		dlist_delete(>node);
-	}
+	/*
+	 * Remove TXN from its containing list. Note that following line covers
+	 * two cases.
+	 *
+	 * If TXN is a subxact we delete it from list of known subxacts. NB:
+	 * nsubxacts count of parent becomes too high in this case.
+	 *
+	 * Otherwise we delete TXN from ordered list of toplevel TXNs.
+	 *
+	 */
+	dlist_delete(>node);
 
 	/* now remove reference from buffer */
 	hash_search(rb->by_txn,


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Robert Haas]

On Tue, Apr 5, 2016 at 10:38 AM, Alvaro Herrera
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Alvaro Herrera]

Simon Riggs wrote:
> On 5 April 2016 at 10:12, Andres Freund  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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Simon Riggs]

On 5 April 2016 at 10:12, Andres Freund  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.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Andres Freund]

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...


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Aleksander Alekseev]

> 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/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Yet another small patch - reorderbuffer.c:1099

[Michael Paquier]

On Tue, Apr 5, 2016 at 1:03 AM, Aleksander Alekseev
 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(>node);
> }
> /* delete from LSN ordered list of toplevel TXNs */
> else
> {
> dlist_delete(>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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers