public inbox for [email protected]  

help / color / mirror / Atom feed

From: Drouvot, Bertrand <[email protected]>
To: Masahiko Sawada <[email protected]>
To: Kyotaro Horiguchi <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Oh, Mike <[email protected]>
Subject: Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
Date: Mon, 11 Oct 2021 11:44:21 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAD21AoD00wV4gt-53ze+ZB8n4bqJrdH8J_UnDHddy8S2A+a25g@mail.gmail.com>
References: <CAD21AoCxVNYK0R=SSPchcx1wm6mx=qJ5W8qDVakFCOAy1UxqAQ@mail.gmail.com>
	<[email protected]>
	<CAD21AoAn-k6OpZ6HSAH_G91tpTXR6KYvkf663kg6EqW-f6sz1w@mail.gmail.com>
	<[email protected]>
	<CAD21AoD00wV4gt-53ze+ZB8n4bqJrdH8J_UnDHddy8S2A+a25g@mail.gmail.com>

Hi,

On 10/11/21 8:27 AM, Masahiko Sawada wrote:
> On Fri, Oct 8, 2021 at 4:50 PM Kyotaro Horiguchi
> <[email protected]> wrote:
>> At Thu, 7 Oct 2021 13:20:14 +0900, Masahiko Sawada <[email protected]> wrote in
>>> Another idea to fix this problem would be that before calling
>>> SnapBuildCommitTxn() we create transaction entries in ReorderBuffer
>>> for (sub)transactions whose COMMIT record has XACT_XINFO_HAS_INVALS,
>>> and then mark all of them as catalog-changed by calling
>>> ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for
>>> this idea. What the patch does is essentially the same as what the
>>> proposed patch does. But the patch doesn't modify the
>>> SnapBuildCommitTxn(). And we remember the list of last running
>>> transactions in reorder buffer and the list is periodically purged
>>> during decoding RUNNING_XACTS records, eventually making it empty.
>> I came up with the third way.  SnapBuildCommitTxn already properly
>> handles the case where a ReorderBufferTXN with
>> RBTXN_HAS_CATALOG_CHANGES.  So this issue can be resolved by create
>> such ReorderBufferTXNs in SnapBuildProcessRunningXacts.
> Thank you for the idea and patch!

Thanks you both for your new patches proposal!

I liked Sawada's one but also do "prefer" Horiguchi's one.

>
> It's much simpler than mine. I think that creating an entry of a
> catalog-changed transaction in the reorder buffer before
> SunapBuildCommitTxn() is the right direction.
+1
>
> After more thought, given DDLs are not likely to happen than DML in
> practice, probably we can always mark both the top transaction and its
> subtransactions as containing catalog changes if the commit record has
> XACT_XINFO_HAS_INVALS? I believe this is not likely to lead to
> overhead in practice. That way, the patch could be more simple and
> doesn't need the change of AssertTXNLsnOrder().
>
> I've attached another PoC patch. Also, I've added the tests for this
> issue in test_decoding.

Thanks!

It looks good to me, just have a remark about the comment:

+   /*
+    * Mark the top transaction and its subtransactions as containing 
catalog
+    * changes, if the commit record has invalidation message. This is 
necessary
+    * for the case where we decode only the commit record of the 
transaction
+    * that actually has done catalog changes.
+    */

What about?

     /*
      * Mark the top transaction and its subtransactions as containing 
catalog
      * changes, if the commit record has invalidation message. This is 
necessary
      * for the case where we did not decode the transaction that did 
the catalog
      * change(s) (the decoding restarted after). So that we are 
decoding only the
      * commit record of the transaction that actually has done catalog 
changes.
      */

Thanks

Bertrand

view thread (136+ messages)  latest in thread

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox