From: Maxim Orlov <m.orlov@postgrespro.ru>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Greg Nancarrow <gregn4422@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
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: Tue, 13 Jul 2021 13:10:34 +0300
Message-ID: <150825360832da68872abb4d891dfc3a@postgrespro.ru> (raw)
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>
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.
Attachments:v4-0001-PG14-Fix-parallel-worker-failed-assertion-and-coredump.patch (text/x-diff, 1.5K) downloaddiff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.cindex 3550ef13baa..6443d492501 100644--- a/src/backend/access/transam/parallel.c+++ b/src/backend/access/transam/parallel.c@@ -34,6 +34,7 @@
#include "pgstat.h"
#include "storage/ipc.h"
#include "storage/predicate.h"
+#include "storage/procarray.h"
#include "storage/sinval.h"
#include "storage/spin.h"
#include "tcop/tcopprot.h"
@@ -45,6 +46,7 @@
#include "utils/snapmgr.h"
#include "utils/typcache.h"
+
/*
* We don't want to waste a lot of memory on an error queue which, most of
* the time, will process only a handful of small messages. However, it is
@@ -1261,6 +1263,7 @@ ParallelWorkerMain(Datum main_arg)
char *uncommittedenumsspace;
StringInfoData msgbuf;
char *session_dsm_handle_space;
+ Snapshot asnap;
/* Set flag to indicate that we're initializing a parallel worker. */
InitializingParallelWorker = true;
@@ -1415,7 +1418,15 @@ ParallelWorkerMain(Datum main_arg)
/* Restore active snapshot. */
asnapspace = shm_toc_lookup(toc, PARALLEL_KEY_ACTIVE_SNAPSHOT, false);
- PushActiveSnapshot(RestoreSnapshot(asnapspace));+ asnap = RestoreSnapshot(asnapspace)+ PushActiveSnapshot(asnap);++ /*+ * We may have different xmins in active and transaction snapshots in+ * parallel workers. So, use minimum for TransactionXmin.+ */+ if (TransactionIdFollows(TransactionXmin, asnap->xmin))+ ProcArrayInstallRestoredXmin(p->xmin, fps->parallel_leader_pgproc);
/*
* We've changed which tuples we can see, and must therefore invalidate
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: m.orlov@postgrespro.ru, tomas.vondra@enterprisedb.com, pashkin.elfe@gmail.com, gregn4422@gmail.com, robertmhaas@gmail.com, michael@paquier.xyz, pengchengliu@tju.edu.cn, andres@anarazel.de
Subject: Re: Parallel scan with SubTransGetTopmostTransaction assert coredump
In-Reply-To: <150825360832da68872abb4d891dfc3a@postgrespro.ru>
* 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