Subject: Re: Global snapshots
To: Alexey Kondratov <a.kondratov@postgrespro.ru>,
 "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Cc: tsunakawa.takay@fujitsu.com, movead.li@highgo.ca,
 'Amit Kapila' <amit.kapila16@gmail.com>,
 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
 PostgreSQL-Dev <pgsql-hackers@postgresql.org>
References: <07b2c899-4ed0-4c87-1327-23c750311248@postgrespro.ru>
 <9964cf46-9294-34b9-4858-971e9029f5c7@postgrespro.ru>
 <CAA4eK1Jo=1261+qEKmm69Bm9wv6fvR7kVbqDo7EFUw3vkQtC7w@mail.gmail.com>
 <f23083b9-38d0-6126-eb6e-091816a78585@postgrespro.ru>
 <CAA4eK1K8ibXN8vknTYvjz1sd+JaDDohvV9jBxSs+=1=KJz=u=w@mail.gmail.com>
 <CA+fd4k6oZtO-MFYmunHVecGaTWre8YKDNTSfX9hZhQh6Kui1kA@mail.gmail.com>
 <CAA4eK1JVsMWUD4q-b+vawehFzJb6Qg0AOGq7qOGL_gm6EEhdJg@mail.gmail.com>
 <CA+fd4k6HE8xLGEvqWzABEg8kkju5MxU+if7bf-md0_2pjzXp9Q@mail.gmail.com>
 <CAA4eK1+tMDXU9WtQ0kRgyk1jPr+K-7FowMdFB8=t-ptUEQa-mQ@mail.gmail.com>
 <CA+fd4k6YTa25VxpsqvpvqZmsCErT5T8=MYNNDXt7Wn0DsJvSrg@mail.gmail.com>
 <CAA4eK1KAB=YbvDd3a8g2qNj2QNrQANnrpNmoaNCtEMLfaiFjrg@mail.gmail.com>
 <TYAPR01MB2990E83F9E886A9C8AB8E269FE760@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <TYAPR01MB29902D8B671A5A6946F5AEA8FE720@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <806c3708-26ea-b94d-87b1-d48e41e93726@postgrespro.ru>
 <3ef7877bfed0582019eab3d462a43275@postgrespro.ru>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <057004b6-68bf-98ee-7c57-91c418f6e221@oss.nttdata.com>
Date: Tue, 8 Sep 2020 11:49:36 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <3ef7877bfed0582019eab3d462a43275@postgrespro.ru>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2020/09/05 3:31, Alexey Kondratov wrote:
> Hi,
> 
> On 2020-07-27 09:44, Andrey V. Lepikhov wrote:
>> On 7/27/20 11:22 AM, tsunakawa.takay@fujitsu.com wrote:
>>>
>>> US8356007B2 - Distributed transaction management for database systems with multiversioning - Google Patents
>>> https://patents.google.com/patent/US8356007
>>>
>>>
>>> If it is, can we circumvent this patent?
>>>
>>
>> Thank you for the research (and previous links too).
>> I haven't seen this patent before. This should be carefully studied.
> 
> I had a look on the patch set, although it is quite outdated, especially on 0003.
> 
> Two thoughts about 0003:
> 
> First, IIUC atomicity of the distributed transaction in the postgres_fdw is achieved by the usage of 2PC. I think that this postgres_fdw 2PC support should be separated from global snapshots.

Agreed.


> It could be useful to have such atomic distributed transactions even without a proper visibility, which is guaranteed by the global snapshot. Especially taking into account the doubts about Clock-SI and general questions about algorithm choosing criteria above in the thread.
> 
> Thus, I propose to split 0003 into two parts and add a separate GUC 'postgres_fdw.use_twophase', which could be turned on independently from 'postgres_fdw.use_global_snapshots'. Of course if the latter is enabled, then 2PC should be forcedly turned on as well.
> 
> Second, there are some problems with errors handling in the 0003 (thanks to Arseny Sher for review).
> 
> +error:
> +            if (!res)
> +            {
> +                sql = psprintf("ABORT PREPARED '%s'", fdwTransState->gid);
> +                BroadcastCmd(sql);
> +                elog(ERROR, "Failed to PREPARE transaction on remote node");
> +            }
> 
> It seems that we should never reach this point, just because BroadcastStmt will throw an ERROR if it fails to prepare transaction on the foreign server:
> 
> +            if (PQresultStatus(result) != expectedStatus ||
> +                (handler && !handler(result, arg)))
> +            {
> +                elog(WARNING, "Failed command %s: status=%d, expected status=%d", sql, PQresultStatus(result), expectedStatus);
> +                pgfdw_report_error(ERROR, result, entry->conn, true, sql);
> +                allOk = false;
> +            }
> 
> Moreover, It doesn't make much sense to try to abort prepared xacts, since if we failed to prepare it somewhere, then some foreign servers may become unavailable already and this doesn't provide us a 100% guarantee of clean up.
> 
> +    /* COMMIT open transaction of we were doing 2PC */
> +    if (fdwTransState->two_phase_commit &&
> +        (event == XACT_EVENT_PARALLEL_COMMIT || event == XACT_EVENT_COMMIT))
> +    {
> +        BroadcastCmd(psprintf("COMMIT PREPARED '%s'", fdwTransState->gid));
> +    }
> 
> At this point, the host (local) transaction is already committed and there is no way to abort it gracefully. However, BroadcastCmd may rise an ERROR that will cause a PANIC, since it is non-recoverable state:
> 
> PANIC:  cannot abort transaction 487, it was already committed
> 
> Attached is a patch, which implements a plain 2PC in the postgres_fdw and adds a GUC 'postgres_fdw.use_twophase'. Also it solves these errors handling issues above and tries to add proper comments everywhere. I think, that 0003 should be rebased on the top of it, or it could be a first patch in the set, since it may be used independently. What do you think?

Thanks for the patch!

Sawada-san was proposing another 2PC patch at [1]. Do you have any thoughts
about pros and cons between your patch and Sawada-san's?

Regards,

[1]
https://www.postgresql.org/message-id/CA+fd4k4z6_B1ETEvQamwQhu4RX7XsrN5ORL7OhJ4B5B6sW-RgQ@mail.gmail.com


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION