pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Pavel Borisov <pashkin.elfe@gmail.com>
To: Greg Nancarrow <gregn4422@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Maxim Orlov <m.orlov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Pengchengliu <pengchengliu@tju.edu.cn>
Cc: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Parallel scan with SubTransGetTopmostTransaction assert coredump
Date: Fri, 9 Jul 2021 19:36:22 +0200
Message-ID: <5d730e4e-4fb2-bd5c-f6be-488ad692ed3a@enterprisedb.com> (raw )
In-Reply-To: <CALT9ZEGzD+a8BdYMirNS0stf_qi4uV=hAnK4k3G=Pb37KVWcyQ@mail.gmail.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 >
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
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
view thread (62+ messages) latest in thread
Message-ID: <5d730e4e-4fb2-bd5c-f6be-488ad692ed3a@enterprisedb.com>
Permalink: ../5d730e4e-4fb2-bd5c-f6be-488ad692ed3a@enterprisedb.com/
Also on: postgresql.org/message-id/5d730e4e-4fb2-bd5c-f6be-488ad692ed3a@enterprisedb.com
copy link · copy postgr.es
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, pashkin.elfe@gmail.com, gregn4422@gmail.com, robertmhaas@gmail.com, m.orlov@postgrespro.ru, michael@paquier.xyz, pengchengliu@tju.edu.cn, andres@anarazel.de
Subject: Re: Parallel scan with SubTransGetTopmostTransaction assert coredump
In-Reply-To: <5d730e4e-4fb2-bd5c-f6be-488ad692ed3a@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox