Horton

Thread

Commits

  1. Drop slot's LWLock before returning from SaveSlotToPath()

  1. potential stuck lock in SaveSlotToPath()

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-03-18T15:46:23Z

    When SaveSlotToPath() is called with elevel=LOG, the early exits don't 
release the slot's io_in_progress_lock.  Fix attached.

This could result in a walsender being stuck on the lock forever.  A 
possible way to get into this situation is if the offending code paths 
are triggered in a low disk space situation.  (This is how it was found; 
maybe there are other ways.)

Pavan Deolasee and Craig Ringer worked on this issue.  I'm forwarding it 
on their behalf.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: potential stuck lock in SaveSlotToPath()

    Andres Freund <andres@anarazel.de> — 2020-03-18T18:45:42Z

    Hi,

On 2020-03-18 16:46:23 +0100, Peter Eisentraut wrote:
> When SaveSlotToPath() is called with elevel=LOG, the early exits don't
> release the slot's io_in_progress_lock.  Fix attached.

I'm a bit confused as to why we we ever call it with elevel = LOG
(i.e. why we have the elevel parameter at all). That seems to have been
there from the start, so it's either me or Robert that's to blame. But I
can't immediately see a reason for it?

Greetings,

Andres Freund

  3. Re: potential stuck lock in SaveSlotToPath()

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-18T19:54:19Z

    On 2020-Mar-18, Andres Freund wrote:

> Hi,
> 
> On 2020-03-18 16:46:23 +0100, Peter Eisentraut wrote:
> > When SaveSlotToPath() is called with elevel=LOG, the early exits don't
> > release the slot's io_in_progress_lock.  Fix attached.
> 
> I'm a bit confused as to why we we ever call it with elevel = LOG
> (i.e. why we have the elevel parameter at all). That seems to have been
> there from the start, so it's either me or Robert that's to blame. But I
> can't immediately see a reason for it?

I guess you didn't want failure to save a slot be a reason to abort a
checkpoint.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: potential stuck lock in SaveSlotToPath()

    Thomas Munro <thomas.munro@gmail.com> — 2020-03-18T20:13:28Z

    On Thu, Mar 19, 2020 at 4:46 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> [patch]

+         * releaseing even in that case.

Typo.

  5. Re: potential stuck lock in SaveSlotToPath()

    Andres Freund <andres@anarazel.de> — 2020-03-18T20:25:30Z

    Hi,

On 2020-03-18 16:54:19 -0300, Alvaro Herrera wrote:
> On 2020-Mar-18, Andres Freund wrote:
> > On 2020-03-18 16:46:23 +0100, Peter Eisentraut wrote:
> > > When SaveSlotToPath() is called with elevel=LOG, the early exits don't
> > > release the slot's io_in_progress_lock.  Fix attached.
> > 
> > I'm a bit confused as to why we we ever call it with elevel = LOG
> > (i.e. why we have the elevel parameter at all). That seems to have been
> > there from the start, so it's either me or Robert that's to blame. But I
> > can't immediately see a reason for it?
> 
> I guess you didn't want failure to save a slot be a reason to abort a
> checkpoint.

I don't see a valid reason for that though - if anything it's dangerous,
because we're not persistently saving the slot. It should fail the
checkpoint imo. Robert, do you have an idea?

Greetings,

Andres Freund

  6. Re: potential stuck lock in SaveSlotToPath()

    Robert Haas <robertmhaas@gmail.com> — 2020-03-19T15:38:28Z

    On Wed, Mar 18, 2020 at 4:25 PM Andres Freund <andres@anarazel.de> wrote:
> I don't see a valid reason for that though - if anything it's dangerous,
> because we're not persistently saving the slot. It should fail the
> checkpoint imo. Robert, do you have an idea?

Well, the comment atop SaveSlotToPath says:

 * This needn't actually be part of a checkpoint, but it's a convenient
 * location.

And I agree with that.

Incidentally, the wait-event handling in SaveSlotToPath() doesn't look
right for the early-exit cases either.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  7. Re: potential stuck lock in SaveSlotToPath()

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-03-20T15:32:47Z

    On 2020-03-19 16:38, Robert Haas wrote:
> Incidentally, the wait-event handling in SaveSlotToPath() doesn't look
> right for the early-exit cases either.

There appear to be appropriate pgstat_report_wait_end() calls.  What are 
you seeing?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: potential stuck lock in SaveSlotToPath()

    Robert Haas <robertmhaas@gmail.com> — 2020-03-20T15:38:15Z

    On Fri, Mar 20, 2020 at 11:32 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 2020-03-19 16:38, Robert Haas wrote:
> > Incidentally, the wait-event handling in SaveSlotToPath() doesn't look
> > right for the early-exit cases either.
>
> There appear to be appropriate pgstat_report_wait_end() calls.  What are
> you seeing?

Oh, you're right. I think I got confused because the rename() and
close() don't have that, but those don't have a wait event set either.
Sorry for the noise.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: potential stuck lock in SaveSlotToPath()

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-03-25T10:13:19Z

    On 2020-03-20 16:38, Robert Haas wrote:
> On Fri, Mar 20, 2020 at 11:32 AM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 2020-03-19 16:38, Robert Haas wrote:
>>> Incidentally, the wait-event handling in SaveSlotToPath() doesn't look
>>> right for the early-exit cases either.
>>
>> There appear to be appropriate pgstat_report_wait_end() calls.  What are
>> you seeing?
> 
> Oh, you're right. I think I got confused because the rename() and
> close() don't have that, but those don't have a wait event set either.
> Sorry for the noise.

Any concerns about applying and backpatching the patch I posted?

The talk about reorganizing this code doesn't seem very concrete at the 
moment and would probably not be backpatch material anyway.


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: potential stuck lock in SaveSlotToPath()

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-25T13:41:13Z

    On 2020-Mar-25, Peter Eisentraut wrote:

> On 2020-03-20 16:38, Robert Haas wrote:
> > On Fri, Mar 20, 2020 at 11:32 AM Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> wrote:
> > > On 2020-03-19 16:38, Robert Haas wrote:
> > > > Incidentally, the wait-event handling in SaveSlotToPath() doesn't look
> > > > right for the early-exit cases either.
> > > 
> > > There appear to be appropriate pgstat_report_wait_end() calls.  What are
> > > you seeing?
> > 
> > Oh, you're right. I think I got confused because the rename() and
> > close() don't have that, but those don't have a wait event set either.
> > Sorry for the noise.
> 
> Any concerns about applying and backpatching the patch I posted?

It looks a straight bug fix to me, I agree it should be back-patched.

> The talk about reorganizing this code doesn't seem very concrete at the
> moment and would probably not be backpatch material anyway.

Agreed on both counts.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: potential stuck lock in SaveSlotToPath()

    Robert Haas <robertmhaas@gmail.com> — 2020-03-25T16:56:10Z

    On Wed, Mar 25, 2020 at 6:13 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Any concerns about applying and backpatching the patch I posted?

Not from me.

> The talk about reorganizing this code doesn't seem very concrete at the
> moment and would probably not be backpatch material anyway.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  12. Re: potential stuck lock in SaveSlotToPath()

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-03-26T13:16:05Z

    On 2020-03-25 17:56, Robert Haas wrote:
> On Wed, Mar 25, 2020 at 6:13 AM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Any concerns about applying and backpatching the patch I posted?
> 
> Not from me.
> 
>> The talk about reorganizing this code doesn't seem very concrete at the
>> moment and would probably not be backpatch material anyway.
> 
> +1.

committed and backpatched

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  13. Re: potential stuck lock in SaveSlotToPath()

    Michael Paquier <michael@paquier.xyz> — 2020-03-27T07:48:45Z

    On Thu, Mar 26, 2020 at 02:16:05PM +0100, Peter Eisentraut wrote:
> committed and backpatched

The patch committed does that in three places:
    /* rename to permanent file, fsync file and directory */
    if (rename(tmppath, path) != 0)
    {
+       LWLockRelease(&slot->io_in_progress_lock);
	ereport(elevel,
                (errcode_for_file_access(),
                 errmsg("could not rename file \"%s\" to \"%s\": %m",

But why do you assume that LWLockRelease() never changes errno?  It
seems to me that you should save errno before calling LWLockRelease(),
and then restore it back before using %m in the log message, no?  See
for example the case where trace_lwlocks is set.
--
Michael

  14. Re: potential stuck lock in SaveSlotToPath()

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-01T14:26:25Z

    On 2020-03-27 08:48, Michael Paquier wrote:
> On Thu, Mar 26, 2020 at 02:16:05PM +0100, Peter Eisentraut wrote:
>> committed and backpatched
> 
> The patch committed does that in three places:
>      /* rename to permanent file, fsync file and directory */
>      if (rename(tmppath, path) != 0)
>      {
> +       LWLockRelease(&slot->io_in_progress_lock);
> 	ereport(elevel,
>                  (errcode_for_file_access(),
>                   errmsg("could not rename file \"%s\" to \"%s\": %m",
> 
> But why do you assume that LWLockRelease() never changes errno?  It
> seems to me that you should save errno before calling LWLockRelease(),
> and then restore it back before using %m in the log message, no?  See
> for example the case where trace_lwlocks is set.

Good catch.  How about the attached patch?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  15. Re: potential stuck lock in SaveSlotToPath()

    Michael Paquier <michael@paquier.xyz> — 2020-04-02T06:21:50Z

    On Wed, Apr 01, 2020 at 04:26:25PM +0200, Peter Eisentraut wrote:
> Good catch.  How about the attached patch?

WFM.  Another trick would be to call LWLockRelease() after generating
the log, but I find your patch more consistent with the surroundings.
--
Michael

  16. Re: potential stuck lock in SaveSlotToPath()

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-05T08:13:44Z

    On 2020-04-02 08:21, Michael Paquier wrote:
> On Wed, Apr 01, 2020 at 04:26:25PM +0200, Peter Eisentraut wrote:
>> Good catch.  How about the attached patch?
> 
> WFM.  Another trick would be to call LWLockRelease() after generating
> the log, but I find your patch more consistent with the surroundings.

done

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services