On Wed, Oct 21, 2020 at 11:31:54AM -0400, Robert Haas wrote: > On Sat, Oct 17, 2020 at 7:53 AM Noah Misch <n...@leadboat.com> wrote: > > Let's move InvalidateSystemCaches() later. > > Invalidation should follow any worker initialization step that changes the > > results of relcache validation; otherwise, we'd need to ensure the > > InvalidateSystemCaches() will not validate any relcache entry. > > The thinking behind the current placement was this: just before the > call to InvalidateSystemCaches(), we restore the active and > transaction snapshots. I think that we must now flush the caches > before anyone does any more lookups; otherwise, they might get wrong > answers. So, putting this code later makes the assumption that no such > lookups happen meanwhile. That feels a little risky to me; at the very > least, it should be clearly spelled out in the comments that no system > cache lookups can happen in the functions we call in the interim.
My comment edits did attempt that. I could enlarge those comments, at the risk of putting undue weight on the topic. One could also arrange for an assertion failure if something takes a snapshot in the unwelcome period, between StartParallelWorkerTransaction() and InvalidateSystemCaches(). Looking closer, we have live bugs from lookups during RestoreGUCState(): $ echo "begin; create user alice; set session authorization alice; set force_parallel_mode = regress; select 1" | psql -X BEGIN CREATE ROLE SET SET ERROR: role "alice" does not exist CONTEXT: while setting parameter "session_authorization" to "alice" $ echo "begin; create text search configuration foo (copy=english); set default_text_search_config = foo; set force_parallel_mode = regress; select 1" | psql -X BEGIN CREATE TEXT SEARCH CONFIGURATION SET SET ERROR: invalid value for parameter "default_text_search_config": "public.foo" CONTEXT: while setting parameter "default_text_search_config" to "public.foo" I gather $SUBJECT is the tip of an iceberg, so I'm withdrawing the patch and abandoning the topic.
