On Fri, Jan 10, 2020 at 11:56:37AM +0530, Mahendra Singh Thalor wrote:
> I reviewed and tested the patch. After applying patch, I am getting other
> assert failure.
>
> I think, before committing 1st patch, we should fix this crash also and
> then we should commit all the patches.
I have somewhat managed to break my environment for a couple of days
so as I got zero testing done with assertions, so I missed this one.
Thanks for the lookup! The environment is fixed since.
This code path uses an assertion that would become incorrect once you
are able to create in pg_class temporary relations which rely on a
temporary schema that does not exist anymore, because its schema has
been dropped it, and that's what you are doing. The assertion does
not concern only autovacuum originally, as it would fail each time a
session tries to build a relation descriptor for its cache with a
relation using a non-existing namespace. I have not really dug if
that's actually possible to trigger.. Anyway.
So, on the one hand, saying that we allow orphaned temporary relations
to be dropped even if their schema does not exist is what autovacuum
does now more aggressively, so that can help to avoid having to clean
up yourself orphaned entries from catalogs, following up with their
toast entries, etc. And this approach makes the assertion lose its
meaning for autovacuum.
On the other hand keeping this assertion makes sure that we never try
to load incorrect relcache entries, and just make autovacuum less
aggressive by ignoring orphaned entries with incorrect namespace
references, though the user experience in fixing the cluster means
manual manipulation of the catalogs. This is something I understood
we'd like to avoid as much as possible, while keeping autovacuum
aggressive on the removal as that can ease the life of people fixing a
cluster. So this would bring us back to a point intermediate of
246a6c8.
This makes me wonder how much we should try to outsmart somebody which
puts the catalogs in such a inconsistent state. Hmm. Perhaps at the
end autovacuum should just ignore such entries and just don't help the
user at all as this also comes with its own issues with the storage
level as well as smgr.c uses rd_backend. And if the user plays with
temporary namespaces like that with superuser rights, he likely knows
what he is doing. Perhaps not :D, in which case autovacuum may not be
the best thing to decide that. I still think we should make the log
of autovacuum.c for orphaned relations more careful with its coding
though, and fix it with the previous patch. The documentation of
isTempNamespaceInUse() could gain in clarity, just a nit from me while
looking at the surroundings. And actually I found an issue with its
logic, as the routine would not consider a temp namespace in use for a
session's own MyBackendId. As that's only used for autovacuum, this
has no consequence, but let's be correct in hte long run.
And this gives the attached after a closer lookup. Thoughts?
--
Michael
