Обсуждение: Re: SQL function which allows to distinguish a server being in point in time recovery mode and an ordinary replica

On Tue Mar 26, 2024 at 9:28 AM CDT, m.litsarev wrote:
> Hi,
>
> At present time, an existing pg_is_in_recovery() method is not enough
> to distinguish a server being in point in time recovery (PITR) mode and
> an ordinary replica
> because it returns true in both cases.
>
> That is why pg_is_standby_requested() function introduced in attached
> patch might help.
> It reports whether a standby.signal file was found in the data directory
> at startup process.
> Instructions for reproducing the possible use case are also attached.
>
> Hope it will be usefull.

Hey Mikhail,

Saw your patch for the first time today. Looks like your patch is messed
up? You seem to have more of the diff at the bottom which seems to add
a test. Want to send a v2 with a properly formatted patch?

Example command:

git format-patch -v2 -M HEAD^

--
Tristan Partin
Neon (https://neon.tech)

On Mon, Apr 15, 2024 at 04:06:03PM -0500, Tristan Partin wrote:
> Saw your patch for the first time today. Looks like your patch is messed up?
> You seem to have more of the diff at the bottom which seems to add a test.
> Want to send a v2 with a properly formatted patch?

FWIW, complicating more XLogRecoveryCtlData sends me shivers, these
days, because we have already a lot of recovery state to track within
it.

More seriously, I'm not much a fan of introducing more branches at the
bottom of readRecoverySignalFile() for the boolean flags tracking if
standby and/or archive recovery are triggered, even if these are
simple there are already too many of them.  Perhaps we should begin
tracking all that as a set of bitmasks, then plug in the tracked state
in shmem for consumption in some SQL function.
--
Michael

On 2024-Apr-16, Michael Paquier wrote:

> there are already too many of them.  Perhaps we should begin
> tracking all that as a set of bitmasks, then plug in the tracked state
> in shmem for consumption in some SQL function.

Yes, it sounds reasonable.
Let me implement some initial draft and come back with it after a while.

Respectfully,

Mikhail Litsarev
Postgres Professional: https://postgrespro.com

> simple there are already too many of them.  Perhaps we should begin
> tracking all that as a set of bitmasks, then plug in the tracked state
> in shmem for consumption in some SQL function.

Hi!

Michael, Tristan
as a first step I have introduced the `SharedRecoveryDataFlags` bitmask 
instead of three boolean SharedHotStandbyActive, 
SharedPromoteIsTriggered and SharedStandbyModeRequested flags (the last 
one from my previous patch) and made minimal updates in corresponding 
code based on that change.

Respectfully,

Mikhail Litsarev
Postgres Professional: https://postgrespro.com

On Mon, May 06, 2024 at 06:55:46PM +0300, m.litsarev@postgrespro.ru wrote:
> as a first step I have introduced the `SharedRecoveryDataFlags` bitmask
> instead of three boolean SharedHotStandbyActive, SharedPromoteIsTriggered
> and SharedStandbyModeRequested flags (the last one from my previous patch)
> and made minimal updates in corresponding code based on that change.

Thanks for the patch.

 /*
- * Local copy of SharedHotStandbyActive variable. False actually means "not
+ * Local copy of XLR_HOT_STANDBY_ACTIVE flag. False actually means "not
  * known, need to check the shared state".
  */
 static bool LocalHotStandbyActive = false;

 /*
- * Local copy of SharedPromoteIsTriggered variable. False actually means "not
+ * Local copy of XLR_PROMOTE_IS_TRIGGERED flag. False actually means "not
  * known, need to check the shared state".
  */
 static bool LocalPromoteIsTriggered = false;

It's a bit strange to have a bitwise set of flags in shmem while we
keep these local copies as booleans.  Perhaps it would be cleaner to
merge both local variables into a single bits32 store?

+   uint32      SharedRecoveryDataFlags;

I'd switch to bits32 for flags here.

+bool
+StandbyModeIsRequested(void)
+{
+    /*
+     * Spinlock is not needed here because XLR_STANDBY_MODE_REQUESTED flag
+     * can only be read after startup process is done.
+     */
+    return (XLogRecoveryCtl->SharedRecoveryDataFlags & XLR_STANDBY_MODE_REQUESTED) != 0;
+}

How about introducing a single wrapper function that returns the whole
value SharedRecoveryDataFlags, with the flags published in a header?
Sure, XLR_HOT_STANDBY_ACTIVE is not really exciting because being able
to query a standby implies it, but XLR_PROMOTE_IS_TRIGGERED could be
interesting?  Then this could be used with a function that returns a
text[] array with all the states retrieved?

The refactoring pieces and the function pieces should be split, for
clarity.
--
Michael