From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas@vondra.me>
Cc: Sergey Belyashov <sergey.belyashov@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18815: Logical replication worker Segmentation fault
Date: Mon, 17 Feb 2025 20:51:50 -0500
Message-ID: <1147221.1739843510@sss.pgh.pa.us> (raw)
In-Reply-To: <1072645.1739835476@sss.pgh.pa.us>
References: <18815-2a0407cc7f40b327@postgresql.org>
<832225.1739810351@sss.pgh.pa.us>
<CAOe0RDwbowSGv8qeuqyHzJndtwV5JyPEG4YwmaAvgquC=gjz+Q@mail.gmail.com>
<906545.1739821437@sss.pgh.pa.us>
<911898.1739823180@sss.pgh.pa.us>
<917262.1739825850@sss.pgh.pa.us>
<1072645.1739835476@sss.pgh.pa.us>
I wrote:
> The problem seems to be that apply_handle_insert_internal does
> ExecOpenIndices and then ExecCloseIndices, and then
> ExecCleanupTupleRouting does ExecCloseIndices again, which nicely
> explains why brininsertcleanup blows up if you happen to have a BRIN
> index involved. What it doesn't explain is how come we don't see
> other symptoms from the duplicate index_close calls, regardless of
> index type.
Hah, I see it: this bug is specific to the apply_handle_tuple_routing
code path. The ResultRelInfo we're dealing with is made by
ExecFindPartition, which does ExecOpenIndices on it. Then
apply_handle_tuple_routing calls apply_handle_insert_internal, which
does ExecOpenIndices *again* on the same ResultRelInfo. That gets a
second refcount and second lock on the index(es), and leaks all the
IndexInfo data structures made by the first call. When done,
apply_handle_insert_internal does ExecCloseIndices, releasing one
refcount and lock. Then, back in apply_handle_tuple_routing, we do
ExecCloseIndices again. So the refcounts and locks balance out, and
it very accidentally fails to crash, so long as nobody is expecting
the contents of the IndexInfos to match up.
The "Move the tuple into the new partition" path in
apply_handle_tuple_routing's UPDATE case is even squirrelier. That
does another ExecFindPartition and then apply_handle_insert_internal,
but there's no ExecCloseIndices call balancing the ExecFindPartition,
meaning it looks like it's leaking refcount and lock. Experimentation
shows that it's not, because the matching ExecCloseIndices is
eventually done by the ExecCleanupTupleRouting call in finish_edata
after returning to the outer apply_handle_update.
So if you ask me, this is impossibly convoluted and in need of a
significant rewrite. It's not okay to be opening/closing the
ResultRelInfo's indexes twice (not least because "speculative"
is different in the two open calls). And it's not okay to have
such radically different paths for cleaning up the index-opening
done by ExecFindPartition. It's even more not okay that there's
not one word of commentary about this complexity, strongly suggesting
that the original author didn't quite understand how it worked either.
regards, tom lane
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-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, tomas@vondra.me, sergey.belyashov@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18815: Logical replication worker Segmentation fault
In-Reply-To: <1147221.1739843510@sss.pgh.pa.us>
* 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