public inbox for [email protected]  

help / color / mirror / Atom feed

From: [email protected] <[email protected]>
To: 'Oh, Mike' <[email protected]>
To: [email protected] <[email protected]>
Cc: Drouvot, Bertrand <[email protected]>
Subject: RE: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
Date: Tue, 5 Oct 2021 07:37:23 +0000
Message-ID: <OSBPR01MB4888111BDF767C34E9DF5D82EDAF9@OSBPR01MB4888.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <OSBPR01MB488815590FF2EE07B612A394EDA49@OSBPR01MB4888.jpnprd01.prod.outlook.com>
References: <[email protected]>
	<[email protected]>
	<OSBPR01MB488815590FF2EE07B612A394EDA49@OSBPR01MB4888.jpnprd01.prod.outlook.com>

On Friday, September 24, 2021 5:03 PM I wrote:
> On Tuesday, March 16, 2021 1:35 AM Oh, Mike <[email protected]> wrote:
> > We noticed that the logical replication could fail when the
> > Standby::RUNNING_XACT record is generated in the middle of a catalog
> > modifying transaction and if the logical decoding has to restart from
> > the RUNNING_XACT WAL entry.
> ...
> > Proposed solution:
> > If we’re decoding a catalog modifying commit record, then check
> > whether it’s part of the RUNNING_XACT xid’s processed @ the
> > restart_lsn. If so, then add its xid & subxacts in the committed txns
> > list in the logical decoding snapshot.
> >
> > Please refer to the attachment for the proposed patch.
> 
> 
> Let me share some review comments for the patch.
....
> (3) suggestion of small readability improvement
> 
> We calculate the same size twice here and DecodeCommit.
> I suggest you declare a variable that stores the computed result of size, which
> might shorten those codes.
> 
> +                                       /*
> +                                        * xl_running_xacts contains a xids
> Flexible Array
> +                                        * and its size is subxcnt + xcnt.
> +                                        * Take that into account while
> allocating
> +                                        * the memory for last_running.
> +                                        */
> +                                       last_running = (xl_running_xacts *)
> malloc(sizeof(xl_running_xacts)
> +
> + sizeof(TransactionId )
> +
> * (running->subxcnt + running->xcnt));
> +                                       memcpy(last_running, running,
> sizeof(xl_running_xacts)
> +
> + (sizeof(TransactionId)
> +
> + * (running->subxcnt + running->xcnt)));
Let me add one more basic review comment in DecodeStandbyOp().

Why do you call raw malloc directly ?
You don't have the basic check whether the return value is
NULL or not and intended to call palloc here instead ?


Best Regards,
	Takamichi Osumi

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]
  Subject: RE: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
  In-Reply-To: <OSBPR01MB4888111BDF767C34E9DF5D82EDAF9@OSBPR01MB4888.jpnprd01.prod.outlook.com>

* 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