On Wed, Oct 21, 2020 at 11:31:54AM -0400, Robert Haas wrote:
> On Sat, Oct 17, 2020 at 7:53 AM Noah Misch <noah@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.