MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_1e8aae32afca14b52ae0cb0d28524359"
Content-Transfer-Encoding: 7bit
Date: Tue, 13 Jul 2021 13:10:34 +0300
From: Maxim Orlov <m.orlov@postgrespro.ru>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Greg Nancarrow
 <gregn4422@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Michael Paquier
 <michael@paquier.xyz>, Pengchengliu <pengchengliu@tju.edu.cn>, Andres Freund
 <andres@anarazel.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Parallel scan with SubTransGetTopmostTransaction assert coredump
In-Reply-To: <5d730e4e-4fb2-bd5c-f6be-488ad692ed3a@enterprisedb.com>
References: 
 <CAJcOf-cJfLcgxwz8TeTZqWe3z2XdLkZAT8gHTwgHqHP9-Cb-Mw@mail.gmail.com>
 <CAJcOf-eDcGVUx0+HjeYzj73AK+x2OOLEQLcfPn4ThEG9pwZpSw@mail.gmail.com>
 <000901d74b84$fa262890$ee7279b0$@tju.edu.cn>
 <CAJcOf-dD-428NkhDKm8GKTDVU97LYUeOd=RazhLR9boxO_ywsA@mail.gmail.com>
 <000801d74d16$150494c0$3f0dbe40$@tju.edu.cn>
 <CAJcOf-dVeKS2YJaS_BVdF7ghQ=xFj2d68TzGjYCvzo8cM0FtDQ@mail.gmail.com>
 <CAJcOf-c63QJ3X7geemcjtkd7ohHM1=7xJYuv8W6LLRAc9DNvOQ@mail.gmail.com>
 <YKswhdCNo6gwn2w2@paquier.xyz>
 <CAJcOf-cNLhA7iaUYAQqZ44tz3oHJoPxGRm1+tNE27iJXTXObzQ@mail.gmail.com>
 <CALT9ZEG4vzW1C=U4-AvgCA9=QEqVfrxaQ8PQ98CGNZ5LBG96kQ@mail.gmail.com>
 <CAJcOf-dX8ufSeCghz9Wg4CrtpJboCxtkqjtK6giLowkwaROcuQ@mail.gmail.com>
 <09788181118f7bd8000d3bb097e0720f@postgrespro.ru>
 <08ae178b309c617a9701cbc7ed8a98f0@postgrespro.ru>
 <CAJcOf-engk+dyWJcdZzNZct1aGG2gqTbPzorDeTARKxrP89QAQ@mail.gmail.com>
 <CALT9ZEFXVCjXOLonRe0GGV_B14i8txsNP3LDXo2-4QraSryCgQ@mail.gmail.com>
 <CALT9ZEGzD+a8BdYMirNS0stf_qi4uV=hAnK4k3G=Pb37KVWcyQ@mail.gmail.com>
 <5d730e4e-4fb2-bd5c-f6be-488ad692ed3a@enterprisedb.com>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <150825360832da68872abb4d891dfc3a@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/150825360832da68872abb4d891dfc3a%40postgrespro.ru>
Precedence: bulk

--=_1e8aae32afca14b52ae0cb0d28524359
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

On 2021-07-09 20:36, Tomas Vondra wrote:
> Hi,
> 
> I took a quick look on this - I'm no expert in the details of 
> snapshots,
> so take my comments with a grain of salt.
> 
> AFAICS both Greg Nancarrow and Pavel Borisov are kinda right. I think
> Greg is right the v3 patch does not seem like the right (or correct)
> solution, for a couple reasons:
> 
> 
> 1) It fixes the symptoms, not the cause. If we set TransactionXmin to a
> bogus value, this only fixes it locally in 
> SubTransGetTopmostTransaction
> but I'd be surprised if other places did not have the same issue.
> 
> 
> 2) The xid/TransactionXmin comparison in the v2 fix:
> 
>   xid = xid > TransactionXmin ? xid : TransactionXmin;
> 
> seems broken, because it compares the XIDs directly, but that's not
> going to work after generating enough transactions.
> 
> 
> 3) But even if this uses TransactionIdFollowsOrEquals, it seems very
> strange because the "xid" comes from
> 
>   XidInMVCCSnapshot(HeapTupleHeaderGetRawXmin(tuple), snapshot))
> 
> i.e. it's the raw xmin from the tuple, so why should we be setting it 
> to
> TransactionXmin? That seems pretty strange, TBH.
> 
> 
> So yeah, I think this is due to confusion with two snapshots and 
> failing
> to consider both of them when calculating TransactionXmin.
> 
> But I think removing one of the snapshots (as the v2 does it) is rather
> strange too. I very much doubt having both the transaction and active
> snapshots in the parallel worker is not intentional, and Pavel may very
> well be right this breaks isolation levels that use the xact snapshot
> (i.e. REPEATABLE READ and SERIALIZABLE). I haven't checked, though.
> 
> So I think we need to keep both snapshots, but fix TransactionXmin to
> consider both of them - I suppose ParallelWorkerMain could simply look
> at the two snapshots, and use the minimum. Which is what [1] (already
> linked by Pavel) talks about, although that's related to concerns about
> one of the processes dying, which is not what's happening here.
> 
> 
> I'm wondering what consequences this may have on production systems,
> though. We've only seen this failing because of the assert, so what
> happens when the build has asserts disabled?
> 
> Looking at SubTransGetTopmostTransaction, it seems the while loop ends
> immediately (because it's pretty much the opposite of the assert), so 
> we
> just return the "xid" as topmost XID. But will that produce incorrect
> query results, or what?
> 
> 
> 
> regards
> 
> 
> [1]
> https://www.postgresql.org/message-id/20150208002027.GH9201%40alap3.anarazel.de

PFA v4 patch based on the ideas above.

In principle I see little difference with v3. But I agree it is more 
correct.

I did test this patch on Linux and MacOS using testing algo above and 
got no error. On master branch before the patch I still see this error.

---
Best regards,
Maxim Orlov.
--=_1e8aae32afca14b52ae0cb0d28524359
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=v4-0001-PG14-Fix-parallel-worker-failed-assertion-and-coredump.patch
Content-Disposition: attachment;
 filename*0*=UTF-8''v4-0001-PG14-Fix-parallel-worker-failed-assertion-and-;
 filename*1*=coredump.patch;
 size=1587

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3BhcmFsbGVsLmMgYi9zcmMv
YmFja2VuZC9hY2Nlc3MvdHJhbnNhbS9wYXJhbGxlbC5jCmluZGV4IDM1NTBlZjEzYmFhLi42NDQz
ZDQ5MjUwMSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0vcGFyYWxsZWwu
YworKysgYi9zcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNhbS9wYXJhbGxlbC5jCkBAIC0zNCw2ICsz
NCw3IEBACiAjaW5jbHVkZSAicGdzdGF0LmgiCiAjaW5jbHVkZSAic3RvcmFnZS9pcGMuaCIKICNp
bmNsdWRlICJzdG9yYWdlL3ByZWRpY2F0ZS5oIgorI2luY2x1ZGUgInN0b3JhZ2UvcHJvY2FycmF5
LmgiCiAjaW5jbHVkZSAic3RvcmFnZS9zaW52YWwuaCIKICNpbmNsdWRlICJzdG9yYWdlL3NwaW4u
aCIKICNpbmNsdWRlICJ0Y29wL3Rjb3Bwcm90LmgiCkBAIC00NSw2ICs0Niw3IEBACiAjaW5jbHVk
ZSAidXRpbHMvc25hcG1nci5oIgogI2luY2x1ZGUgInV0aWxzL3R5cGNhY2hlLmgiCiAKKwogLyoK
ICAqIFdlIGRvbid0IHdhbnQgdG8gd2FzdGUgYSBsb3Qgb2YgbWVtb3J5IG9uIGFuIGVycm9yIHF1
ZXVlIHdoaWNoLCBtb3N0IG9mCiAgKiB0aGUgdGltZSwgd2lsbCBwcm9jZXNzIG9ubHkgYSBoYW5k
ZnVsIG9mIHNtYWxsIG1lc3NhZ2VzLiAgSG93ZXZlciwgaXQgaXMKQEAgLTEyNjEsNiArMTI2Myw3
IEBAIFBhcmFsbGVsV29ya2VyTWFpbihEYXR1bSBtYWluX2FyZykKIAljaGFyCSAgICp1bmNvbW1p
dHRlZGVudW1zc3BhY2U7CiAJU3RyaW5nSW5mb0RhdGEgbXNnYnVmOwogCWNoYXIJICAgKnNlc3Np
b25fZHNtX2hhbmRsZV9zcGFjZTsKKwlTbmFwc2hvdAlhc25hcDsKIAogCS8qIFNldCBmbGFnIHRv
IGluZGljYXRlIHRoYXQgd2UncmUgaW5pdGlhbGl6aW5nIGEgcGFyYWxsZWwgd29ya2VyLiAqLwog
CUluaXRpYWxpemluZ1BhcmFsbGVsV29ya2VyID0gdHJ1ZTsKQEAgLTE0MTUsNyArMTQxOCwxNSBA
QCBQYXJhbGxlbFdvcmtlck1haW4oRGF0dW0gbWFpbl9hcmcpCiAKIAkvKiBSZXN0b3JlIGFjdGl2
ZSBzbmFwc2hvdC4gKi8KIAlhc25hcHNwYWNlID0gc2htX3RvY19sb29rdXAodG9jLCBQQVJBTExF
TF9LRVlfQUNUSVZFX1NOQVBTSE9ULCBmYWxzZSk7Ci0JUHVzaEFjdGl2ZVNuYXBzaG90KFJlc3Rv
cmVTbmFwc2hvdChhc25hcHNwYWNlKSk7CisJYXNuYXAgPSBSZXN0b3JlU25hcHNob3QoYXNuYXBz
cGFjZSkKKwlQdXNoQWN0aXZlU25hcHNob3QoYXNuYXApOworCisJLyoKKwkgKiBXZSBtYXkgaGF2
ZSBkaWZmZXJlbnQgeG1pbnMgaW4gYWN0aXZlIGFuZCB0cmFuc2FjdGlvbiBzbmFwc2hvdHMgaW4K
KwkgKiBwYXJhbGxlbCB3b3JrZXJzLiBTbywgdXNlIG1pbmltdW0gZm9yIFRyYW5zYWN0aW9uWG1p
bi4KKwkgKi8KKwlpZiAoVHJhbnNhY3Rpb25JZEZvbGxvd3MoVHJhbnNhY3Rpb25YbWluLCBhc25h
cC0+eG1pbikpCisJCVByb2NBcnJheUluc3RhbGxSZXN0b3JlZFhtaW4ocC0+eG1pbiwgZnBzLT5w
YXJhbGxlbF9sZWFkZXJfcGdwcm9jKTsKIAogCS8qCiAJICogV2UndmUgY2hhbmdlZCB3aGljaCB0
dXBsZXMgd2UgY2FuIHNlZSwgYW5kIG11c3QgdGhlcmVmb3JlIGludmFsaWRhdGUK
--=_1e8aae32afca14b52ae0cb0d28524359--