Horton

Thread

Commits

  1. Tweak pg_promote() to report failures on kill() or postmaster failures

  1. Return value of pg_promote()

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2023-06-06T11:05:58Z

    Hi All,

At present, pg_promote() returns true to the caller on successful
promotion of standby, however it returns false in multiple scenarios
which includes:

1) The SIGUSR1 signal could not be sent to the postmaster process.
2) The postmaster died during standby promotion.
3) Standby couldn't be promoted within the specified wait time.

For an application calling this function, if pg_promote returns false,
it is hard to interpret the reason behind it. So I think we should
*only* allow pg_promote to return false when the server could not be
promoted in the given wait time and in other scenarios it should just
throw an error (FATAL, ERROR ... depending on the type of failure that
occurred). Please let me know your thoughts on this change. thanks.!

--
With Regards,
Ashutosh Sharma.

  2. Re: Return value of pg_promote()

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-06-06T17:00:57Z

    On Tue, 2023-06-06 at 16:35 +0530, Ashutosh Sharma wrote:
> At present, pg_promote() returns true to the caller on successful
> promotion of standby, however it returns false in multiple scenarios
> which includes:
> 
> 1) The SIGUSR1 signal could not be sent to the postmaster process.
> 2) The postmaster died during standby promotion.
> 3) Standby couldn't be promoted within the specified wait time.
> 
> For an application calling this function, if pg_promote returns false,
> it is hard to interpret the reason behind it. So I think we should
> *only* allow pg_promote to return false when the server could not be
> promoted in the given wait time and in other scenarios it should just
> throw an error (FATAL, ERROR ... depending on the type of failure that
> occurred). Please let me know your thoughts on this change. thanks.!

As the original author, I'd say that that sounds reasonable, particularly
in case #1.  If the postmaster dies, we are going to die too, so it
probably doesn't matter much.  But I think an error is certainly also
correct in that case.

Yours,
Laurenz Albe

  3. Re: Return value of pg_promote()

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2023-06-07T16:25:38Z

    
On 2023/06/07 2:00, Laurenz Albe wrote:
> On Tue, 2023-06-06 at 16:35 +0530, Ashutosh Sharma wrote:
>> At present, pg_promote() returns true to the caller on successful
>> promotion of standby, however it returns false in multiple scenarios
>> which includes:
>>
>> 1) The SIGUSR1 signal could not be sent to the postmaster process.
>> 2) The postmaster died during standby promotion.
>> 3) Standby couldn't be promoted within the specified wait time.
>>
>> For an application calling this function, if pg_promote returns false,
>> it is hard to interpret the reason behind it. So I think we should
>> *only* allow pg_promote to return false when the server could not be
>> promoted in the given wait time and in other scenarios it should just
>> throw an error (FATAL, ERROR ... depending on the type of failure that
>> occurred). Please let me know your thoughts on this change. thanks.!
> 
> As the original author, I'd say that that sounds reasonable, particularly
> in case #1.  If the postmaster dies, we are going to die too, so it
> probably doesn't matter much.  But I think an error is certainly also
> correct in that case.

+1

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  4. Re: Return value of pg_promote()

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2023-06-08T11:23:50Z

    On Wed, Jun 7, 2023 at 9:55 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2023/06/07 2:00, Laurenz Albe wrote:
> > On Tue, 2023-06-06 at 16:35 +0530, Ashutosh Sharma wrote:
> >> At present, pg_promote() returns true to the caller on successful
> >> promotion of standby, however it returns false in multiple scenarios
> >> which includes:
> >>
> >> 1) The SIGUSR1 signal could not be sent to the postmaster process.
> >> 2) The postmaster died during standby promotion.
> >> 3) Standby couldn't be promoted within the specified wait time.
> >>
> >> For an application calling this function, if pg_promote returns false,
> >> it is hard to interpret the reason behind it. So I think we should
> >> *only* allow pg_promote to return false when the server could not be
> >> promoted in the given wait time and in other scenarios it should just
> >> throw an error (FATAL, ERROR ... depending on the type of failure that
> >> occurred). Please let me know your thoughts on this change. thanks.!
> >
> > As the original author, I'd say that that sounds reasonable, particularly
> > in case #1.  If the postmaster dies, we are going to die too, so it
> > probably doesn't matter much.  But I think an error is certainly also
> > correct in that case.
>
> +1
>

Thanks for sharing your thoughts, Laurenz and Fujii-san. I've prepared
a patch that makes pg_promote error out if it couldn't send SIGUSR1 to
the postmaster or if the postmaster died in the middle of standby
promotion. PFA. Please note that now (with this patch) pg_promote only
returns false if the standby could not be promoted within the given
wait time. In case of any kind of failure, it just reports an error
based on the type of failure that occurred.

--
With Regards,
Ashutosh Sharma.

  5. Re: Return value of pg_promote()

    Michael Paquier <michael@paquier.xyz> — 2023-08-16T08:02:09Z

    On Thu, Jun 08, 2023 at 04:53:50PM +0530, Ashutosh Sharma wrote:
> Thanks for sharing your thoughts, Laurenz and Fujii-san. I've prepared
> a patch that makes pg_promote error out if it couldn't send SIGUSR1 to
> the postmaster or if the postmaster died in the middle of standby
> promotion. PFA. Please note that now (with this patch) pg_promote only
> returns false if the standby could not be promoted within the given
> wait time. In case of any kind of failure, it just reports an error
> based on the type of failure that occurred.

     if (kill(PostmasterPid, SIGUSR1) != 0)
     {
-        ereport(WARNING,
-                (errmsg("failed to send signal to postmaster: %m")));
         (void) unlink(PROMOTE_SIGNAL_FILE);
-        PG_RETURN_BOOL(false);
+        ereport(ERROR,
+                (errmsg("failed to send signal to postmaster: %m")));
     }

Shouldn't you assign an error code to this one rather than the
default one for internal errors, like ERRCODE_SYSTEM_ERROR?

     /* return immediately if waiting was not requested */
@@ -744,7 +743,9 @@ pg_promote(PG_FUNCTION_ARGS)
          * necessity for manual cleanup of all postmaster children.
          */
         if (rc & WL_POSTMASTER_DEATH)
-            PG_RETURN_BOOL(false);
+            ereport(FATAL,
+                    (errcode(ERRCODE_ADMIN_SHUTDOWN),
+                     errmsg("terminating connection due to unexpected postmaster exit")));

I would add an errcontext here, to let somebody know that the
connection died while waiting for the promotion to be processed, say
"while waiting on promotion".
--
Michael

  6. Re: Return value of pg_promote()

    Michael Paquier <michael@paquier.xyz> — 2023-08-17T00:37:36Z

    On Wed, Aug 16, 2023 at 05:02:09PM +0900, Michael Paquier wrote:
>      if (kill(PostmasterPid, SIGUSR1) != 0)
>      {
> -        ereport(WARNING,
> -                (errmsg("failed to send signal to postmaster: %m")));
>          (void) unlink(PROMOTE_SIGNAL_FILE);
> -        PG_RETURN_BOOL(false);
> +        ereport(ERROR,
> +                (errmsg("failed to send signal to postmaster: %m")));
>      }
> 
> Shouldn't you assign an error code to this one rather than the
> default one for internal errors, like ERRCODE_SYSTEM_ERROR?
> 
>      /* return immediately if waiting was not requested */
> @@ -744,7 +743,9 @@ pg_promote(PG_FUNCTION_ARGS)
>           * necessity for manual cleanup of all postmaster children.
>           */
>          if (rc & WL_POSTMASTER_DEATH)
> -            PG_RETURN_BOOL(false);
> +            ereport(FATAL,
> +                    (errcode(ERRCODE_ADMIN_SHUTDOWN),
> +                     errmsg("terminating connection due to unexpected postmaster exit")));
> 
> I would add an errcontext here, to let somebody know that the
> connection died while waiting for the promotion to be processed, say
> "while waiting on promotion".

I have just noticed that we do not have a CF entry for this proposal,
so I have added one with Laurenz as author:
https://commitfest.postgresql.org/44/4504/

For now the patch is waiting on author.  Could you address my
last review?
--
Michael

  7. Re: Return value of pg_promote()

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2023-08-28T06:20:45Z

    Hi Michael,

On Thu, Aug 17, 2023 at 6:07 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Aug 16, 2023 at 05:02:09PM +0900, Michael Paquier wrote:
> >      if (kill(PostmasterPid, SIGUSR1) != 0)
> >      {
> > -        ereport(WARNING,
> > -                (errmsg("failed to send signal to postmaster: %m")));
> >          (void) unlink(PROMOTE_SIGNAL_FILE);
> > -        PG_RETURN_BOOL(false);
> > +        ereport(ERROR,
> > +                (errmsg("failed to send signal to postmaster: %m")));
> >      }
> >
> > Shouldn't you assign an error code to this one rather than the
> > default one for internal errors, like ERRCODE_SYSTEM_ERROR?
> >
> >      /* return immediately if waiting was not requested */
> > @@ -744,7 +743,9 @@ pg_promote(PG_FUNCTION_ARGS)
> >           * necessity for manual cleanup of all postmaster children.
> >           */
> >          if (rc & WL_POSTMASTER_DEATH)
> > -            PG_RETURN_BOOL(false);
> > +            ereport(FATAL,
> > +                    (errcode(ERRCODE_ADMIN_SHUTDOWN),
> > +                     errmsg("terminating connection due to unexpected postmaster exit")));
> >
> > I would add an errcontext here, to let somebody know that the
> > connection died while waiting for the promotion to be processed, say
> > "while waiting on promotion".
>
> I have just noticed that we do not have a CF entry for this proposal,
> so I have added one with Laurenz as author:
> https://commitfest.postgresql.org/44/4504/
>
> For now the patch is waiting on author.  Could you address my
> last review?

Thanks for reviewing the patch and adding a CF entry for it. PFA patch
that addresses your review comments.

And... Sorry for the delayed response. I totally missed it.

--
With Regards,
Ashutosh Sharma.

  8. Re: Return value of pg_promote()

    Michael Paquier <michael@paquier.xyz> — 2023-08-28T09:09:44Z

    On Mon, Aug 28, 2023 at 11:50:45AM +0530, Ashutosh Sharma wrote:
> Thanks for reviewing the patch and adding a CF entry for it. PFA patch
> that addresses your review comments.

That looks OK seen from here.  Perhaps others have more comments?

> And... Sorry for the delayed response. I totally missed it.

No problem.
--
Michael

  9. Re: Return value of pg_promote()

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-08-28T12:09:37Z

    On Thu, 2023-08-17 at 09:37 +0900, Michael Paquier wrote:
> I have just noticed that we do not have a CF entry for this proposal,
> so I have added one with Laurenz as author:
> https://commitfest.postgresql.org/44/4504/

I have changed the author to Fujii Masao.

Yours,
Laurenz Albe

  10. Re: Return value of pg_promote()

    Michael Paquier <michael@paquier.xyz> — 2023-08-28T23:47:29Z

    On Mon, Aug 28, 2023 at 02:09:37PM +0200, Laurenz Albe wrote:
> On Thu, 2023-08-17 at 09:37 +0900, Michael Paquier wrote:
> > I have just noticed that we do not have a CF entry for this proposal,
> > so I have added one with Laurenz as author:
> > https://commitfest.postgresql.org/44/4504/
> 
> I have changed the author to Fujii Masao.

Still incorrect, as the author is Ashutosh Sharma.  Fujii-san has
provided some feedback, though.
--
Michael