Subject: Re: out-of-order XID insertion in KnownAssignedXids
To: Michael Paquier <michael@paquier.xyz>, Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <0c96b653-4696-d4b4-6b5d-78143175d113@postgrespro.ru>
 <20181005080422.GA7863@paquier.xyz>
 <581a80f7-3992-f319-302d-7c9a384d64f7@postgrespro.ru>
 <2A819787-9E67-41EF-B89A-A906996924E5@anarazel.de>
 <fc51532c-dbf5-dce0-b31c-82c7a0b837ed@postgrespro.ru>
 <20181008163049.nvaui5kjrsav2ojn@alap3.anarazel.de>
 <20181009055900.GH2137@paquier.xyz> <20181009075258.GI2137@paquier.xyz>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <1e1d56e2-b056-5fa1-bc89-a6bb3836a1c8@postgrespro.ru>
Date: Tue, 9 Oct 2018 17:26:49 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20181009075258.GI2137@paquier.xyz>
Content-Type: multipart/alternative;
 boundary="------------D800BFD1061A8DE9CFA5616C"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------D800BFD1061A8DE9CFA5616C
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit


On 09.10.2018 10:52, Michael Paquier wrote:
> On Tue, Oct 09, 2018 at 02:59:00PM +0900, Michael Paquier wrote:
>> +1.  I am looking at how to make that possible.
> And so...  Going through a bit of investigation the problem is that each
> 2PC transaction preparing finishes by putting the procarray in a state
> where there are two entries referring to the same transaction running as
> MarkAsPrepared adds an extra entry on the top of the one already
> existing.  This is expected though per the comments in
> ProcArrayClearTransaction(), as it assumes that MyProc can be safely
> cleaned and it assumes that there is a duplicated entry.  Then,
> GetRunningTransactionData() ignores the assumption that those duplicate
> entries can exist, which makes the snapshot data taken as incorrect,
> generating those incorrect XLOG_RUNNING_XACT records.
>
> The most simple fix I can think of first is to tweak the origin of the
> problem in GetRunningTransactionData() so as those duplicate XIDs are
> ignored if found as there has to be a state between the time
> MarkAsPrepared() inserted the 2PC entry in the procarray and the time
> ProcArrayClearTransaction() gets called.
> --
> Michael

Right now GetRunningTransactionData is used only once in Postgres code - in LogStandbySnapshot to log RUNNING_XACTS record.
So if this situation is not changed, there will be no difference whether to perform sort and exclude duplicates in LogStandbySnapsho
or GetRunningTransactionData.

But GetRunningTransactionData may be used in some other cases... For example I have used it in my snapfs Postgres extensions (fast file-level snapshots)
to check if there are no more active transactions in the system. In my case presence of duplicates is not important and ordering of XIDs is not needed.
But performance of GetRunningTransactionData is also not critical. But I can suspect that there can be cases when it is critical and doing unneeded qsort is not desired.
There may be thousands of active transactions and sorting their XIDs is notinstantaneous.

Also please take in account that in most cases XIDs in RUNNINGS_XACTS record are not processed at all (only if we perform recovery from backup and do not reach consistent point yet). This is why this bug was not detected before.

So I completely understand the argument that it is better to eliminate source of the problem (presence of duplicates in RUNNING_XACTS record)
but not sure that in this particular case it is really the best solution. If presence of duplicates is considered to be acceptable for procarray, why we
can not accept it for RUNNING_XACTS record?

I am not insisting that skipping duplicates in 
ProcArrayApplyRecoveryInfo is the right place (but at least is seems to 
be the most efficient alternative). But I also do not fill that moving 
sort to GetRunningTransactionData and elimination of duplicates here 
(requires one more scan and copying of the whole array) is principally 
better...


-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


--------------D800BFD1061A8DE9CFA5616C
Content-Type: text/html; charset=windows-1252
Content-Transfer-Encoding: 7bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html;
      charset=windows-1252">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <br>
    <br>
    <div class="moz-cite-prefix">On 09.10.2018 10:52, Michael Paquier
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:20181009075258.GI2137@paquier.xyz">
      <pre wrap="">On Tue, Oct 09, 2018 at 02:59:00PM +0900, Michael Paquier wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="">+1.  I am looking at how to make that possible.
</pre>
      </blockquote>
      <pre wrap="">
And so...  Going through a bit of investigation the problem is that each
2PC transaction preparing finishes by putting the procarray in a state
where there are two entries referring to the same transaction running as
MarkAsPrepared adds an extra entry on the top of the one already
existing.  This is expected though per the comments in
ProcArrayClearTransaction(), as it assumes that MyProc can be safely
cleaned and it assumes that there is a duplicated entry.  Then,
GetRunningTransactionData() ignores the assumption that those duplicate
entries can exist, which makes the snapshot data taken as incorrect,
generating those incorrect XLOG_RUNNING_XACT records.

The most simple fix I can think of first is to tweak the origin of the
problem in GetRunningTransactionData() so as those duplicate XIDs are
ignored if found as there has to be a state between the time
MarkAsPrepared() inserted the 2PC entry in the procarray and the time
ProcArrayClearTransaction() gets called.
--
Michael
</pre>
    </blockquote>
    <br>
    <pre wrap="">Right now GetRunningTransactionData is used only once in Postgres code - in LogStandbySnapshot to log RUNNING_XACTS record.
So if this situation is not changed, there will be no difference whether to perform sort and exclude duplicates in LogStandbySnapsho
or GetRunningTransactionData.

But GetRunningTransactionData may be used in some other cases... For example I have used it in my snapfs Postgres extensions (fast file-level snapshots)
to check if there are no more active transactions in the system. In my case presence of duplicates is not important and ordering of XIDs is not needed.
But performance of GetRunningTransactionData is also not critical. But I can suspect that there can be cases when it is critical and doing unneeded qsort is not desired.
There may be thousands of active transactions and sorting their XIDs is not <span class="gt-baf-word-clickable">instantaneous.</span>

Also please take in account that in most cases XIDs in RUNNINGS_XACTS record are not processed at all (only if we perform recovery from backup and do not reach consistent point yet). This is why this bug was not detected before.

So I completely understand the argument that it is better to eliminate source of the problem (presence of duplicates in RUNNING_XACTS record)
but not sure that in this particular case it is really the best solution. If presence of duplicates is considered to be acceptable for procarray, why we
can not accept it for RUNNING_XACTS record?
</pre>
    I am not insisting that skipping duplicates in
    ProcArrayApplyRecoveryInfo is the right place (but at least is seems
    to be the most efficient alternative). But I also do not fill that
    moving sort to GetRunningTransactionData
    and elimination of duplicates here (requires one more scan and
    copying of the whole array) is principally better...<br>
    <br>
    <br>
    <pre class="moz-signature" cols="72">-- 
Konstantin Knizhnik
Postgres Professional: <a class="moz-txt-link-freetext" href="http://www.postgrespro.com">http://www.postgrespro.com</a>
The Russian Postgres Company </pre>
  </body>
</html>

--------------D800BFD1061A8DE9CFA5616C--