Re: [BUGS] Breakage with VACUUM ANALYSE + partitions - Mailing list pgsql-hackers

On 2016-04-25 16:29:36 -0400, Robert Haas wrote:
> On Mon, Apr 25, 2016 at 2:05 PM, Andres Freund <andres@anarazel.de> wrote:
> > Well, I posted a patch. I'd have applied it too (after addressing your
> > comments obviously), except that there's some interdependencies with the
> > nsmg > 0 thread (some of my tests fail spuriously without that
> > fixed). Early last week I waited for a patch on that thread, but when
> > that didn't materialize by Friday I switched to work on that [1].  With
> > both fixes applied I can't reproduce any problems anymore.
> 
> OK.  What are the interdependencies?  You've said that a few times but
> I am not clear on what the nature of those interdependencies is.

I added checks to smgr/md.c that verify that the smgr state is
consistent with the on-file state. Without the two issues in [1] fixed,
these tests fail in a standby, while running regression tests.  Adding
those tests made me notice a bug in an unreleased version of the patch,
so it seems they're worthwhile to run.


> > About the delay: Sure, it'd be nicer if I'd addressed this
> > immediately. But priority-wise it's an issue that's damned hard to hit,
> > and where the worst known consequence is having to reconnect; that's not
> > earth shattering. So spending time to work on the, imo more severe
> > performance regressions, seemed to be more important; maybe I was wrong
> > in priorizing things that way.
> 
> Do you mean performance regression related to this patch, or to other
> patches like the atomic buffer pin/unpin stuff?

Other patches, I can't see this having measurable impact.


> >> We have a patch, and that's good, and I have reviewed it and
> >> Thom has tested it, and that's good, too.  But it is not clear whether
> >> you feel confident to commit it or when you might be planning to do
> >> that, so I asked.  Given that this is the open item of longest tenure
> >> and that we're hoping to ship beta soon, why is that out of line?
> >
> > Well, if you'd asked it that way, then I'd not responded the way I have.
> 
> Fair enough, but keep in mind that a few more mildly-phrased emails
> from Noah didn't actually get us to the point where this is fixed, or
> even to a clear explanation of when it might be fixed or why it wasn't
> getting fixed sooner.

Hrmpf. I'd initially missed the first email in the onslought, and the
second email I responded to and posted a patch in the timeframe I'd
promised. I didn't forsee, after we'd initially dismissed a correlation,
that there'd be a connection to the other patch.


> >> That commit introduced a new way to write to blocks that might have in
> >> the meantime been removed, and it failed to make that safe.
> >
> > There's no writing of any blocks involved - the problem is just about
> > opening segments which might or might not exist.
> 
> As I understand it, that commit introduced a backend-private queue;
> when it fills, we issue flush requests for particular blocks.  By the
> time the flush requests are issued, the blocks might have been
> truncated away.  So it's not writing blocks in the sense of write(),
> but persuading the OS to write those blocks to stable storage.

Right.


> >> And in fact I think it's a regression that can be
> >> expected to be a significant operational problem for people if not
> >> fixed, because the circumstances in which it can happen are not very
> >> obscure.  You just need to hold some pending flush requests in your
> >> backend-local queue while some other process truncates the relation,
> >> and boom.
> >
> > I found it quite hard to come up with scenarios to reproduce it. Entire
> > segments have to be dropped, the writes to the to-be-dropped segment
> > have to result in fully dead rows, only few further writes outside the
> > dropped can happen, invalidations may not fix the problem up.  But
> > obviously it should be fixed.
> 
> It's probably a bit tricky to produce a workload where the
> truncated-away block is in some backend's private queue, because the
> queues are very small and it's not exactly clear what sort of workload
> will produce regular relation truncations, and you need both of those
> things to hit this. However, I think it's pretty optimistic to think
> that there won't be people in that situation, partly because we had a
> customer find a bug in an EDB proprietary feature  a very similar
> whose profile is very similar to this feature less than 2 years ago.

I'm obviously not arguing that we shouldn't fix this.


> >> I assume you will be pretty darned unhappy if we end up at #2, so I am
> >> trying to figure out if we can achieve #1.  OK?
> >
> > Ok.
> 
> I'm still not clear on the answer to this.  I think the answer is that
> you think that this patch is OK to commit with some editing, but the
> situation with the nmsgs > 0 patch is not so clear yet because it
> hasn't had any review yet.  And they depend on each other somehow.  Am
> I close?

You are. I'd prefer pushing this after fixes for the two invalidation
issues, because it makes testing easier. But I guess the timeframe there
is unclear enough, that it makes sense to test this patch on top of the
the preliminary fixes, and then push without those.

- Andres