Date: Mon, 4 Mar 2024 20:04:52 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Japin Li <japinli@hotmail.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Cary Huang <cary.huang@highgo.ca>,
	SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Switching XLog source from archive to streaming when primary
 available
Message-ID: <20240305020452.GA3373526@nathanxps13>
References: 
 <CALj2ACUr1ZQ5K=bt4EvCPcVc4RT93-zt_TLjMiERjfhrnULRxQ@mail.gmail.com>
 <CALj2ACW0-Pou-ingOXzbva4Xa54CHH7sTDp7wycs62S+VCg2Zg@mail.gmail.com>
 <CALj2ACUkDENzQZKRSB88K8bb7y-8pcYjuj_4i4Zgb7gqd0Ompg@mail.gmail.com>
 <CALj2ACXFm9AQK2PMYs4sn8uJHBPREdj=2=EtsQ9bTd8AsAgnuw@mail.gmail.com>
 <CALj2ACWoh8Q-bWFmmecAeT9KOHCCyE7GTdaCZYYBgAU01nS0wQ@mail.gmail.com>
 <CALj2ACVjx=RATdk1VRcvo5x21atF5X_XveD-Lis5CTc7JLynVw@mail.gmail.com>
 <ME3P282MB3166BDEACEFC8E4DFFE8B157B6512@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
 <CALj2ACU9O0q_JiGbtjPY4Ue2ax4aFH+x5Htw2nwOXQi0=p4ouQ@mail.gmail.com>
 <ME3P282MB3166D3391A9FC11DD35C05D9B6502@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
 <CALj2ACXV9=q5mXbPTgk9gKPQroxjpgyP7VKQnA1FREQO0iRXiA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALj2ACXV9=q5mXbPTgk9gKPQroxjpgyP7VKQnA1FREQO0iRXiA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20240305020452.GA3373526%40nathanxps13>
Precedence: bulk

cfbot claims that this one needs another rebase.

I've spent some time thinking about this one.  I'll admit I'm a bit worried
about adding more complexity to this state machine, but I also haven't
thought of any other viable approaches, and this still seems like a useful
feature.  So, for now, I think we should continue with the current
approach.

+        fails to switch to stream mode, it falls back to archive mode. If this
+        parameter value is specified without units, it is taken as
+        milliseconds. Default is <literal>5min</literal>. With a lower value

Does this really need to be milliseconds?  I would think that any
reasonable setting would at least on the order of seconds.

+        attempts. To avoid this, it is recommended to set a reasonable value.

I think we might want to suggest what a "reasonable value" is.

+	static bool canSwitchSource = false;
+	bool		switchSource = false;

IIUC "canSwitchSource" indicates that we are trying to force a switch to
streaming, but we are currently exhausting anything that's present in the
pg_wal directory, while "switchSource" indicates that we should force a
switch to streaming right now.  Furthermore, "canSwitchSource" is static
while "switchSource" is not.  Is there any way to simplify this?  For
example, would it be possible to make an enum that tracks the
streaming_replication_retry_interval state?

 			/*
 			 * Don't allow any retry loops to occur during nonblocking
-			 * readahead.  Let the caller process everything that has been
-			 * decoded already first.
+			 * readahead if we failed to read from the current source. Let the
+			 * caller process everything that has been decoded already first.
 			 */
-			if (nonblocking)
+			if (nonblocking && lastSourceFailed)
 				return XLREAD_WOULDBLOCK;

Why do we skip this when "switchSource" is set?

+			/* Reset the WAL source switch state */
+			if (switchSource)
+			{
+				Assert(canSwitchSource);
+				Assert(currentSource == XLOG_FROM_STREAM);
+				Assert(oldSource == XLOG_FROM_ARCHIVE);
+				switchSource = false;
+				canSwitchSource = false;
+			}

How do we know that oldSource is guaranteed to be XLOG_FROM_ARCHIVE?  Is
there no way it could be XLOG_FROM_PG_WAL?

+#streaming_replication_retry_interval = 5min	# time after which standby
+					# attempts to switch WAL source from archive to
+					# streaming replication
+					# in milliseconds; 0 disables

I think we might want to turn this feature off by default, at least for the
first release.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com