public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bertrand Drouvot <[email protected]>
To: cca5507 <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
Date: Mon, 12 Aug 2024 10:35:44 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

Hi,

On Mon, Aug 12, 2024 at 04:34:25PM +0800, cca5507 wrote:
> Hi,
> 
> 
> 4, 5 ===
> 
> 
> &gt; if (SnapBuildCurrentState(builder) < SNAPBUILD_BUILDING_SNAPSHOT ||
> &gt;&nbsp; &nbsp; &nbsp;(SnapBuildCurrentState(builder) == SNAPBUILD_BUILDING_SNAPSHOT &amp;&amp; info != XLOG_HEAP_INPLACE) ||
> &gt;&nbsp; &nbsp; &nbsp;ctx-&gt;fast_forward)
> &gt;&nbsp; &nbsp; &nbsp;return;
> 
> 
> 
> I think during fast forward, we also need handle the xlog that marks a transaction
> as&nbsp;catalog modifying, or the snapshot might lose some transactions?

I think it's fine to skip during fast forward as we are not generating logical
changes. It's done that way in master, in your proposal and in my "if" proposals.
Note that my proposals related to the if conditions are for heap2_decode and
heap_decode (not xact_decode).

> &gt; That way we'd still rely on what's being done in the XLOG_HEAP_INPLACE case
> 
> 
> +		if (SnapBuildCurrentState(builder) &gt;= SNAPBUILD_BUILDING_SNAPSHOT)
> +		{
> +			/* Currently only XLOG_HEAP_INPLACE means a catalog modifying */
> +			if (info == XLOG_HEAP_INPLACE &amp;&amp; TransactionIdIsValid(xid))
> +				ReorderBufferXidSetCatalogChanges(ctx-&gt;reorder, xid, buf-&gt;origptr);
> +		}
> 
> 
> 
> We only call&nbsp;ReorderBufferXidSetCatalogChanges() for the xlog that marks a transaction as&nbsp;catalog
> modifying, and we don't care about the other steps being done in the xlog, so I think the current
> approach is ok.

Yeah, I think your proposal does not do anything wrong. I just prefer to put
everything in a single if condition (as per my proposal) so that we can jump
directly in the appropriate case. I think that way the code is easier to maintain
instead of having to deal with the same info values in distinct places.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

view thread (10+ 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: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
  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