Thread: 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

Hi!

Michael,
I have fixed the patches according to your comments.

> merge both local variables into a single bits32 store?
This is done in v3-0001-Standby-mode-requested.patch

> Then this could be used with a function that returns a
> text[] array with all the states retrieved?
Placed this in the v3-0002-Text-array-sql-wrapper.patch

> The refactoring pieces and the function pieces should be split, for
> clarity.
Sure. I also added the third patch with some tests. Perhaps it would be 
usefull.

Respectfully,

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

On Thu, Jun 13, 2024 at 09:07:42PM +0300, m.litsarev@postgrespro.ru wrote:
> Hi!
>
> Michael,
> I have fixed the patches according to your comments.
>
> > merge both local variables into a single bits32 store?
> This is done in v3-0001-Standby-mode-requested.patch
>
> > Then this could be used with a function that returns a
> > text[] array with all the states retrieved?
> Placed this in the v3-0002-Text-array-sql-wrapper.patch
>
> > The refactoring pieces and the function pieces should be split, for
> > clarity.
> Sure. I also added the third patch with some tests. Perhaps it would be
> usefull.

+     *    --    XLR_PROMOTE_IS_TRIGGERED indicates if a standby promotion
+     *        has been triggered. Protected by info_lck.
+     *
+     *     --    XLR_STANDBY_MODE_REQUESTED indicates if we're in a standby mode
+     *        at start, while recovery mode is on. No info_lck protection.
+     *
+     *    and can be extended in future.

This comment is incorrect for XLR_STANDBY_MODE_REQUESTED?  A startup
we would unlikely be in standby mode, most likely in crash recovery,
then switch to standby mode.

-    LocalPromoteIsTriggered = XLogRecoveryCtl->SharedPromoteIsTriggered;
+    LocalRecoveryDataFlags &= ~XLR_PROMOTE_IS_TRIGGERED;
+    LocalRecoveryDataFlags |=
+        (XLogRecoveryCtl->SharedRecoveryDataFlags & XLR_PROMOTE_IS_TRIGGERED)

Are these complications really needed?  All these flags are false,
then switched to true.  true -> false is not possible.

         StandbyModeRequested = true;
         ArchiveRecoveryRequested = true;
+        XLogRecoveryCtl->SharedRecoveryDataFlags |= XLR_STANDBY_MODE_REQUESTED;

Shouldn't STANDBY_MODE be only used in the local flag, as well as an
ARCHIVE_RECOVERY_REQUESTED?  It looks like this could push a bit more
forward the removal of more of these booleans, with a bit more work..

     return (LocalRecoveryDataFlags & XLR_HOT_STANDBY_ACTIVE);
 }

+
 /*
Some noise lying around.

+    /* Returns bit array as Datum */
+    txt_arr = construct_array_builtin(flags, cnt, TEXTOID);

Yep, that's the correct way to do it.

+is($ret_mode_primary, '{}', "master is not a replica");

The test additions are welcome.  Note that we avoid the word "master",
see 229f8c219f8f.
--
Michael

Hi!

Rebased the patch.

Respectfully,

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

Hi,

Fix an error in the patch.

Respectfully,

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