User-Agent: Cyrus-JMAP/3.5.0-alpha0-206-g078a48fda5-fm-20210226.001-g078a48fd
Mime-Version: 1.0
Message-Id: <6eff9861-b059-4eaa-ae70-e5ff877b6448@www.fastmail.com>
In-Reply-To: <4030089.1616124234@sss.pgh.pa.us>
References: 
 <CAH503wDKdYzyq7U-QJqGn=Gm6XmoK+6_6xTJ-Yn5WSvoHLY1Ww@mail.gmail.com>
 <363cb85c-3a03-9a49-b830-8b645c1241d9@enterprisedb.com>
 <YEdqlL3Gr3rZ/aC8@paquier.xyz>
 <CA+hUKGJtZqpMZtYtJZ2cAGfQT5ELEZGKpnK_TjV-fNeB=rYSNw@mail.gmail.com>
 <4245d284-97b2-4056-96c2-455792d9b564@www.fastmail.com>
 <8740f056-a7c0-ff12-0a50-de5324a928ee@enterprisedb.com>
 <822e3a60-56a1-219d-c473-365751c3e108@enterprisedb.com>
 <a32a7f28-570c-24a2-aa2a-9f7c96abd88b@enterprisedb.com>
 <54663718-1d87-46f2-a27b-e7edacbfae0d@www.fastmail.com>
 <1ec33f3d-ea4c-28c8-502c-3eb0a008a6ac@enterprisedb.com>
 <34226249-c5ba-4780-a1bb-f3433d19716e@www.fastmail.com>
 <05d307fc-8391-4091-b279-1b3214658f10@www.fastmail.com>
 <049a2752-fc99-194d-31b2-b15ffa914cd4@enterprisedb.com>
 <a26a35c6-8816-4c2b-91b9-7a428e430a57@www.fastmail.com>
 <482a6498-7713-e937-c554-7d235c415fba@enterprisedb.com>
 <112f0847-a71!a-eec2-e561-2d39dc26127a@enterprisedb.com>
 <4028905.1616122655@sss.pgh.pa.us>
 <19bc3276-6280-43bd-d868-ab3cb146266a@enter!prisedb.com>
 <4030089.1616124234@sss.pgh.pa.us>
Date: Fri, 19 Mar 2021 11:17:50 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Tom Lane" <tgl@sss.pgh.pa.us>,
 "Tomas Vondra" <tomas.vondra@enterprisedb.com>
Cc: "Thomas Munro" <thomas.munro@gmail.com>,
 "Michael Paquier" <michael@paquier.xyz>,
 "Euler Taveira" <euler.taveira@2ndquadrant.com>,
 "Anastasia Lubennikova" <a.lubennikova@postgrespro.ru>,
 "Tomas Vondra" <tomas.vondra@2ndquadrant.com>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Subject: Re: cleanup temporary files after crash
Content-Type: multipart/mixed;
 boundary=1f63f807d8fb4451a6b264ad8d9543c1
Archived-At: 
 <https://www.postgresql.org/message-id/6eff9861-b059-4eaa-ae70-e5ff877b6448%40www.fastmail.com>
Precedence: bulk

--1f63f807d8fb4451a6b264ad8d9543c1
Content-Type: multipart/alternative;
 boundary=3397fecc8cb94beeb2bbd807fcfe1e41

--3397fecc8cb94beeb2bbd807fcfe1e41
Content-Type: text/plain

On Fri, Mar 19, 2021, at 12:23 AM, Tom Lane wrote:
> [ reads code... ]
> ... no, I think the problem is the test is still full of race conditions.
> 
> In the first place, waiting till you see the output of a SELECT that's
> before the useful query is not enough to guarantee that the useful query
> is done, or even started.  That's broken on both sessions.
That's an ugly and fragile mechanism to workaround the fact that pump_until
reacts after you have the query return.

> In the second place, even if the second INSERT has started, you don't know
> that it's reached the point of blocking on the tuple conflict yet.
> Which in turn means that it might not've filled its tuplestore yet.
> 
> In short, this script is designed to ensure that the test query can't
> finish too soon, but that proves nothing about whether the test query
> has even started.  And since you also haven't really guaranteed that the
> intended-to-be-blocking query is done, I don't think that the first
> condition really holds either.
In order to avoid the race condition between filling the tuplestore and killing
the backend, we could use a pool_query_until() before SIGKILL to wait the
temporary file being created. Do you think this modification will make this
test more stable?


--
Euler Taveira
EDB   https://www.enterprisedb.com/

--3397fecc8cb94beeb2bbd807fcfe1e41
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}
p.MsoNormal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Fri, Ma=
r 19, 2021, at 12:23 AM, Tom Lane wrote:<br></div><blockquote type=3D"ci=
te" id=3D"qt" style=3D""><div>[ reads code... ]<br></div><div>... no, I =
think the problem is the test is still full of race conditions.<br></div=
><div><br></div><div>In the first place, waiting till you see the output=
 of a SELECT that's<br></div><div>before the useful query is not enough =
to guarantee that the useful query<br></div><div>is done, or even starte=
d.&nbsp; That's broken on both sessions.<br></div></blockquote><div>That=
's an ugly and fragile mechanism to workaround the fact that pump_until<=
br></div><div>reacts after you have the query return.<br></div><div><br>=
</div><blockquote type=3D"cite" id=3D"qt" style=3D""><div>In the second =
place, even if the second INSERT has started, you don't know<br></div><d=
iv>that it's reached the point of blocking on the tuple conflict yet.<br=
></div><div>Which in turn means that it might not've filled its tuplesto=
re yet.<br></div><div><br></div><div>In short, this script is designed t=
o ensure that the test query can't<br></div><div>finish too soon, but th=
at proves nothing about whether the test query<br></div><div>has even st=
arted.&nbsp; And since you also haven't really guaranteed that the<br></=
div><div>intended-to-be-blocking query is done, I don't think that the f=
irst<br></div><div>condition really holds either.<br></div></blockquote>=
<div>In order to avoid the race condition between filling the tuplestore=
 and killing<br></div><div>the backend, we could use a pool_query_until(=
) before SIGKILL to wait the<br></div><div>temporary file being created.=
 Do you think this modification will make this<br></div><div>test more s=
table?<br></div><div><br></div><div><br></div><div id=3D"sig112409080"><=
div class=3D"signature">--<br></div><div class=3D"signature">Euler Tavei=
ra<br></div><div class=3D"signature">EDB&nbsp;&nbsp; <a href=3D"https://=
www.enterprisedb.com/">https://www.enterprisedb.com/</a><br></div></div>=
<div><br></div></body></html>
--3397fecc8cb94beeb2bbd807fcfe1e41--

--1f63f807d8fb4451a6b264ad8d9543c1
Content-Disposition: attachment;filename="v3-fix-crash-temp-files.patch"
Content-Type: text/x-patch; name="v3-fix-crash-temp-files.patch"
Content-Transfer-Encoding: BASE64

ZGlmZiAtLWdpdCBhL3NyYy90ZXN0L3JlY292ZXJ5L3QvMDIyX2NyYXNoX3RlbXBfZmlsZXMu
cGwgYi9zcmMvdGVzdC9yZWNvdmVyeS90LzAyMl9jcmFzaF90ZW1wX2ZpbGVzLnBsCmluZGV4
IDgwNDQ4NDliNzMuLjQxYTkxZWJkMDYgMTAwNjQ0Ci0tLSBhL3NyYy90ZXN0L3JlY292ZXJ5
L3QvMDIyX2NyYXNoX3RlbXBfZmlsZXMucGwKKysrIGIvc3JjL3Rlc3QvcmVjb3ZlcnkvdC8w
MjJfY3Jhc2hfdGVtcF9maWxlcy5wbApAQCAtMTAwLDYgKzEwMCwxMSBAQCBvayhwdW1wX3Vu
dGlsKCRraWxsbWUsIFwka2lsbG1lX3N0ZG91dCwgcXIvaW4tcHJvZ3Jlc3MtYmVmb3JlLXNp
Z2tpbGwvbSksCiAka2lsbG1lX3N0ZG91dCA9ICcnOwogJGtpbGxtZV9zdGRlcnIgPSAnJzsK
IAorIyBXYWl0IHRpbGwgYSB0ZW1wb3JhcnkgZmlsZSBpcyBjcmVhdGVkCiskbm9kZS0+cG9s
bF9xdWVyeV91bnRpbCgKKwkncG9zdGdyZXMnLAorCSdTRUxFQ1QgQ09VTlQoMSkgRlJPTSBw
Z19sc19kaXIoJCRiYXNlL3Bnc3FsX3RtcCQkKScsICcxJyk7CisKICMgS2lsbCB3aXRoIFNJ
R0tJTEwKIG15ICRyZXQgPSBUZXN0TGliOjpzeXN0ZW1fbG9nKCdwZ19jdGwnLCAna2lsbCcs
ICdLSUxMJywgJHBpZCk7CiBpcygkcmV0LCAwLCAna2lsbGVkIHByb2Nlc3Mgd2l0aCBLSUxM
Jyk7CkBAIC0xNjgsNiArMTczLDExIEBAIG9rKHB1bXBfdW50aWwoJGtpbGxtZSwgXCRraWxs
bWVfc3Rkb3V0LCBxci9pbi1wcm9ncmVzcy1iZWZvcmUtc2lna2lsbC9tKSwKICRraWxsbWVf
c3Rkb3V0ID0gJyc7CiAka2lsbG1lX3N0ZGVyciA9ICcnOwogCisjIFdhaXQgdGlsbCBhIHRl
bXBvcmFyeSBmaWxlIGlzIGNyZWF0ZWQKKyRub2RlLT5wb2xsX3F1ZXJ5X3VudGlsKAorCSdw
b3N0Z3JlcycsCisJJ1NFTEVDVCBDT1VOVCgxKSBGUk9NIHBnX2xzX2RpcigkJGJhc2UvcGdz
cWxfdG1wJCQpJywgJzEnKTsKKwogIyBLaWxsIHdpdGggU0lHS0lMTAogJHJldCA9IFRlc3RM
aWI6OnN5c3RlbV9sb2coJ3BnX2N0bCcsICdraWxsJywgJ0tJTEwnLCAkcGlkKTsKIGlzKCRy
ZXQsIDAsICdraWxsZWQgcHJvY2VzcyB3aXRoIEtJTEwnKTsK

--1f63f807d8fb4451a6b264ad8d9543c1--