Re: Shaky coding for vacuuming partitioned relations

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Sep 29, 2017 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> If I understand correctly, problem #1 is that get_rel_oids() can emit
>>> a user-visible cache lookup failure message. There is a proposed patch
>>> by Michael Paquier which appears to implement the design suggested by
>>> Tom. I think that the normal procedure would be for Tom to commit
>>> that change if he's happy with it.

>> Yes, I'm happy to take responsibility for this.

> Great, thanks.

After thinking more carefully about the idea I proposed upthread (viz,
take and keep ShareUpdateExclusiveLock), I realized that that will be
pretty horrid once the proposed patch to allow VACUUM to name multiple
target tables goes in. We'd be trying to take a pretty strong lock on
multiple tables at once during get_rel_oids(), creating substantial risk
of deadlock. Up to now, VACUUM has always been careful to not take more
than one such lock at once, and I think we need to preserve that property.

The simplest solution therefore seems to be to just take AccessShareLock
and then release it as soon as we're done adding the rel and its
partitions to the list of target rels. There's little point in taking a
stronger lock here if we are going to end the transaction and drop it
later, and it's a nicely low-risk answer for v10 in any case.

>>> I don't think I understand problem #2. I think the concern is about
>>> reporting the proper relation name when VACUUM cascades from a
>>> partitioned table to its children and then some kind of concurrent DDL
>>> happens, but I don't see a clear explanation on the thread as to what
>>> exactly the failure scenario is, and I didn't see a problem in some
>>> simple tests I just ran.

>> I think the conclusion was that this wouldn't actually happen in v10,
>> but I will take a closer look and do something about it if it is possible.

> Even better, thanks.

After looking closer, I confirmed that there's no live bug today,
though it's a closer shave than one could wish. The RangeVar passed to
vacuum_rel and/or analyze_rel can be wrong, or even NULL, but it is
only used when autovacuum.c requests failure logging, and for that
particular use-case the RangeVar will always be correct.

I felt though that I didn't want to just leave it like that, particularly
in view of the existence of multiple comments claiming things that were
entirely false. So I adjusted those two functions to explicitly allow
for NULL RangeVars, and tried to improve the comments.

Nathan Bossart wants to see logging of relation-open failures in more
cases, and to get that we're going to have to work harder to track
valid RangeVars for target relations. But that is not v10 material.

regards, tom lane