On 9/27/18, 2:52 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Thanks for the new patches. So I have begun looking at the full set,
> beginning by 0001 which introduces a new common routine to get the
> elevel associated to a skipped relation. I have been chewing on this
> one, and I think that the patch could do more in terms of refactoring by
> introducing one single routine able to open a relation which is going to
> be vacuumed or analyzed. This removes quite a lot of duplication
> between analyze_rel() and vacuum_rel() when it comes to using
> try_relation_open(). The result is attached, and that makes the code
> closer to what the recently-added vacuum_is_relation_owner does.
> Nathan, what do you think?
Good idea. This patch looks good to me.
> Please note that I am on the edge of discarding the stuff in
> find_all_inheritors and such, as well as not worrying about the case of
> analyze which could block for some index columns, but I have not spent
> much time studying this part yet. Still the potential issues around
> complicating this code, particularly when things come to having a
> partial analyze or vacuum done rather scares me.
Without the find_all_inheritors() stuff, I think we would just need to
modify the ANALYZE documentation patch to say something like
Specifies that ANALYZE should not wait for any conflicting locks
to be released: if a relation cannot be locked immediately without
waiting, the relation is skipped. Note that even with this
option, ANALYZE can still block when opening the relation's
indexes or when acquiring sample rows to prepare the statistics.
Also, while ANALYZE ordinarily processes all leaf partitions of
partitioned tables, this option will cause ANALYZE to skip all
leaf partitions if there is a conflicting lock on the partitioned
table.
Nathan
