BUG #14208: Inconsistent code modification - 3

Started by petrum@gmail.comalmost 10 years ago10 messagesbugs

almost 10 years ago

The following bug has been logged on the website:

Bug reference: 14208
Logged by: Petru-Florin Mihancea
Email address: petrum@gmail.com
PostgreSQL version: 9.4.4
Operating system: MacOSX
Description:

File: postgresql-9.4.4/src/backend/replication/logical/reorderbuffer.c
Function: ReorderBufferInterTXNInit
Line: 870

The line is

if (txn->nentries != txn->nentries_mem)

But shouldn't be there cur_txn instead of txn?

I do not know exactly the semantics of the code because I detected the
problem with a CodeSonar prototype plugin. However, let me explain why I
think it is a problem:

The line 870 is part of a processing step (lines 866-883) performed for each
sub-transaction of a transaction. Just before this code fragment, the same
processing step is performed for the toplevel transaction (lines 841-857).
The check at 845 is exactly as the one in 870.

However, the buffer of a sub-transaction is cur_txn (and not txn) and thus,
I think cur_txn should be used in 870. Moreover, in the doc comment of
nentries field it is specified that "Changes in subtransactions are *not*
included but tracked separately". Thus again, it looks that the nentries
field for a sub-transaction should be used in line 870 and not the one of
the toplevel transaction.

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

Tom Lane

tgl@sss.pgh.pa.us

almost 10 years ago

In reply to: petrum@gmail.com (#1)

Re: BUG #14208: Inconsistent code modification - 3

petrum@gmail.com writes:

File: postgresql-9.4.4/src/backend/replication/logical/reorderbuffer.c
Function: ReorderBufferInterTXNInit
Line: 870

The line is
if (txn->nentries != txn->nentries_mem)
But shouldn't be there cur_txn instead of txn?

Actually, the function is ReorderBufferIterTXNInit, and in HEAD this
is line 963, but yeah that looks pretty broken. Andres, do you concur?
Or maybe the logic needs to be different for subtransactions?

I do not know exactly the semantics of the code because I detected the
problem with a CodeSonar prototype plugin.

Seems like a cool tool.

regards, tom lane

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

Andres Freund

andres@anarazel.de

almost 10 years ago

In reply to: Tom Lane (#2)

Re: BUG #14208: Inconsistent code modification - 3

On 2016-06-22 11:45:16 -0400, Tom Lane wrote:

petrum@gmail.com writes:

File: postgresql-9.4.4/src/backend/replication/logical/reorderbuffer.c
Function: ReorderBufferInterTXNInit
Line: 870

The line is
if (txn->nentries != txn->nentries_mem)
But shouldn't be there cur_txn instead of txn?

Actually, the function is ReorderBufferIterTXNInit, and in HEAD this
is line 963, but yeah that looks pretty broken. Andres, do you
concur?

Ugh, yes, that looks broken. In a way that can very likely lead to wrong
data being returned :(. I assume an empty toplevel transaction +
subtransactions with spilled-to-disk contents will be bad.

Or maybe the logic needs to be different for subtransactions?

I do not know exactly the semantics of the code because I detected the
problem with a CodeSonar prototype plugin.

Seems like a cool tool.

Indeed. What heuristic lead to detecting this? I can think of some, but
they all owuld have significant false-positive rates.

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

petrum@gmail.com

almost 10 years ago

In reply to: Andres Freund (#3)

Re: BUG #14208: Inconsistent code modification - 3

On 24 Jun 2016, at 01:53, Andres Freund <andres@anarazel.de> wrote:

On 2016-06-22 11:45:16 -0400, Tom Lane wrote:

petrum@gmail.com writes:

File: postgresql-9.4.4/src/backend/replication/logical/reorderbuffer.c
Function: ReorderBufferInterTXNInit
Line: 870

The line is
if (txn->nentries != txn->nentries_mem)
But shouldn't be there cur_txn instead of txn?

Actually, the function is ReorderBufferIterTXNInit, and in HEAD this
is line 963, but yeah that looks pretty broken. Andres, do you
concur?

Ugh, yes, that looks broken. In a way that can very likely lead to wrong
data being returned :(. I assume an empty toplevel transaction +
subtransactions with spilled-to-disk contents will be bad.

Or maybe the logic needs to be different for subtransactions?

I do not know exactly the semantics of the code because I detected the
problem with a CodeSonar prototype plugin.

Seems like a cool tool.

Indeed. What heuristic lead to detecting this? I can think of some, but
they all owuld have significant false-positive rates.

Thank you :). I cannot disclose this. The tool I develop is a CodeSonar (www.grammatech.com)
plugin. It is work in progress still under refinement.

Tom Lane

tgl@sss.pgh.pa.us

almost 10 years ago

In reply to: Andres Freund (#3)

Re: BUG #14208: Inconsistent code modification - 3

Andres Freund <andres@anarazel.de> writes:

On 2016-06-22 11:45:16 -0400, Tom Lane wrote:

Actually, the function is ReorderBufferIterTXNInit, and in HEAD this
is line 963, but yeah that looks pretty broken. Andres, do you
concur?

Ugh, yes, that looks broken. In a way that can very likely lead to wrong
data being returned :(. I assume an empty toplevel transaction +
subtransactions with spilled-to-disk contents will be bad.

Actually, doesn't this mean spilled subtransactions will *always* be lost?
Whether or not the toplevel transaction is empty, by the time we get here
it would have nentries == nentries_mem, no?

Anyway, fix pushed. I did not try to devise a regression test case.

regards, tom lane

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

Andres Freund

andres@anarazel.de

almost 10 years ago

In reply to: Tom Lane (#5)

Re: BUG #14208: Inconsistent code modification - 3

On 2016-06-30 12:40:19 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-06-22 11:45:16 -0400, Tom Lane wrote:

Actually, the function is ReorderBufferIterTXNInit, and in HEAD this
is line 963, but yeah that looks pretty broken. Andres, do you
concur?

Ugh, yes, that looks broken. In a way that can very likely lead to wrong
data being returned :(. I assume an empty toplevel transaction +
subtransactions with spilled-to-disk contents will be bad.

Actually, doesn't this mean spilled subtransactions will *always* be lost?
Whether or not the toplevel transaction is empty, by the time we get here
it would have nentries == nentries_mem, no?

Not, if the top-level transaction spilled to disk.

Anyway, fix pushed.

Thanks!.

I did not try to devise a regression test case.

I'll try to add some.

Andres

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

Tom Lane

tgl@sss.pgh.pa.us

almost 10 years ago

In reply to: Andres Freund (#6)

Re: BUG #14208: Inconsistent code modification - 3

Andres Freund <andres@anarazel.de> writes:

On 2016-06-30 12:40:19 -0400, Tom Lane wrote:

Whether or not the toplevel transaction is empty, by the time we get here
it would have nentries == nentries_mem, no?

Not, if the top-level transaction spilled to disk.

But doesn't the code stanza just above this loop pull that spillage back in?
It's certainly doing *something* to txn->nentries_mem.

regards, tom lane

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

Andres Freund

andres@anarazel.de

almost 10 years ago

In reply to: Tom Lane (#7)

Re: BUG #14208: Inconsistent code modification - 3

On 2016-06-30 12:51:51 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-06-30 12:40:19 -0400, Tom Lane wrote:

Whether or not the toplevel transaction is empty, by the time we get here
it would have nentries == nentries_mem, no?

Not, if the top-level transaction spilled to disk.

But doesn't the code stanza just above this loop pull that spillage
back in?

Do you mean the following?
/* add toplevel transaction if it contains changes */
if (txn->nentries > 0)
{
ReorderBufferChange *cur_change;

if (txn->nentries != txn->nentries_mem)
ReorderBufferRestoreChanges(rb, txn, &state->entries[off].fd,
&state->entries[off].segno);

If so, sure, it pulls changes back in, but only the first
static const Size max_changes_in_memory = 4096;
ones. We should never reconstruct a whole large transaction in memory...

- Andres

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

Tom Lane

tgl@sss.pgh.pa.us

almost 10 years ago

In reply to: Andres Freund (#8)

Re: BUG #14208: Inconsistent code modification - 3

Andres Freund <andres@anarazel.de> writes:

On 2016-06-30 12:51:51 -0400, Tom Lane wrote:

But doesn't the code stanza just above this loop pull that spillage
back in?

If so, sure, it pulls changes back in, but only the first
static const Size max_changes_in_memory = 4096;
ones. We should never reconstruct a whole large transaction in memory...

OK, so the failure case is not "empty top level transaction", but
"top level transaction small enough to not have spilled", plus a
spilled subtransaction, correct?

regards, tom lane

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

#10

Andres Freund

andres@anarazel.de

almost 10 years ago

In reply to: Tom Lane (#9)

Re: BUG #14208: Inconsistent code modification - 3

On June 30, 2016 9:59:07 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-06-30 12:51:51 -0400, Tom Lane wrote:

But doesn't the code stanza just above this loop pull that spillage
back in?

If so, sure, it pulls changes back in, but only the first
static const Size max_changes_in_memory = 4096;
ones. We should never reconstruct a whole large transaction in

memory...

OK, so the failure case is not "empty top level transaction", but
"top level transaction small enough to not have spilled", plus a
spilled subtransaction, correct?

That's how it looks from afar, without having investigated in depth.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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