Date: Tue, 19 Oct 2021 15:43:28 +0900 (JST)
Message-Id: <20211019.154328.1852357242791456526.horikyota.ntt@gmail.com>
To: osumi.takamichi@fujitsu.com
Cc: sawada.mshk@gmail.com, bdrouvot@amazon.com,
 pgsql-hackers@lists.postgresql.org, minsoo@amazon.com
Subject: Re: [BUG] Logical replication failure "ERROR: could not map
 filenode "base/13237/442428" to relation OID" with catalog modifying txns
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: 
 <OSBPR01MB48886D77AC8AD25B16C28B5CEDBD9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
References: 
 <CAD21AoD00wV4gt-53ze+ZB8n4bqJrdH8J_UnDHddy8S2A+a25g@mail.gmail.com>
	<20211014.112128.1398650206608979125.horikyota.ntt@gmail.com>
	<OSBPR01MB48886D77AC8AD25B16C28B5CEDBD9@OSBPR01MB4888.jpnprd01.prod.outlook.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20211019.154328.1852357242791456526.horikyota.ntt%40gmail.com>
Precedence: bulk

At Tue, 19 Oct 2021 02:45:24 +0000, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> wrote in 
> On Thursday, October 14, 2021 11:21 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> > It adds a call to ReorderBufferAssignChild but usually subtransactions are
> > assigned to top level elsewherae.  Addition to that
> > ReorderBufferCommitChild() called just later does the same thing.  We are
> > adding the third call to the same function, which looks a bit odd.
> It can be odd. However, we
> have a check at the top of ReorderBufferAssignChild
> to judge if the sub transaction is already associated or not
> and skip the processings if it is.

My question was why do we need to make the extra call to
ReorerBufferCommitChild when XACT_XINFO_HAS_INVALS in spite of the
existing call to the same fuction that unconditionally made.  It
doesn't cost so much but also it's not free.

> > And I'm not sure it is wise to mark all subtransactions as "catalog changed"
> > always when the top transaction is XACT_XINFO_HAS_INVALS.
> In order to avoid this,
> can't we have a new flag (for example, in reorderbuffer struct) to check
> if we start decoding from RUNNING_XACTS, which is similar to the first patch of [1]
> and use it at DecodeCommit ? This still leads to some extra specific codes added
> to DecodeCommit and this solution becomes a bit similar to other previous patches though.

If it is somehow wrong in any sense that we add subtransactions in
SnapBuildProcessRunningXacts (for example, we should avoid relaxing
the assertion condition.), I think we would go another way.  Otherwise
we don't even need that additional flag.  (But Sawadasan's recent PoC
also needs that relaxation.)

ASAICS, and unless I'm missing something (that odds are rtlatively
high:p), we need the specially added subransactions only for the
transactions that were running at passing the first RUNNING_XACTS,
becuase otherwise (substantial) subtransactions are assigned to
toplevel by the first record of the subtransaction.

Before reaching consistency, DecodeCommit feeds the subtransactions to
ReorderBufferForget individually so the subtransactions are not needed
to be assigned to the top transaction at all. Since the
subtransactions added by the first RUNNING_XACT are processed that
way, we don't need in the first place to call ReorderBufferCommitChild
for such subtransactions.

> [1] - https://www.postgresql.org/message-id/81D0D8B0-E7C4-4999-B616-1E5004DBDCD2%40amazon.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center