On Sat, 2023-07-29 at 21:51 -0700, Nathan Bossart wrote:
> On Sat, Jul 29, 2023 at 08:59:01AM -0700, Jeff Davis wrote:
> > 0001: Transform the settings in proconfig into a List for faster
> > processing. This is simple and speeds up any proconfig setting.
>
> This looks straightforward. It might be worth combining the common
> parts
> of ProcessGUCArray() and TransformGUCArray() if there is a clean way
> to do
> so.
I changed the former to call the latter to share code. A few extra
allocations in the ProcessGUCArray() path, but it doesn't look like
that would matter.
> > 0002: Introduce CheckIdentifierString(), which is a faster version
> > of
> > SplitIdentifierString() that only validates, and can be used in
> > check_search_path().
>
> This also looks straightforward. And I'd make the same comment as
> above
> for SplitIdentifierString() and CheckIdentifierString(). Perhaps we
> could
> have folks set SplitIdentifierString's namelist argument to NULL to
> indicate that it only needs to validate.
SplitIdentifierString() modifies its input, and it doesn't seem like a
net win in readability if the API sometimes modifies its input
(requiring a copy in the caller) and sometimes does not. I'm open to
suggestion about a refactor here, but I didn't see a clean way to do
so.
>
> > One behavior change in 0003 is that retrieving a cached OID list
> > doesn't call InvokeNamespaceSearchHook(). It would be easy enough
> > to
> > disable caching when a hook exists, but I didn't see a reason to
> > expect
> > that "SET search_path" must invoke that hook each time. Invoking it
> > when computing for the first time, or after a real invalidation,
> > seemed
> > fine to me. Feedback on that is welcome.
>
> Could a previously cached list be used to circumvent a hook that was
> just
> reconfigured to ERROR for certain namespaces? If something like that
> is
> possible, then we might need to disable the cache when there is a
> hook.
I changed it to move the hook so that it's called after retrieving from
the cache. Most of the speedup is still there with no behavior change.
It also means I had to move the deduplication to happen after
retrieving from the cache, which doesn't seem great but in practice the
search path list is not very long so I don't think it will make much
difference. (Arguably, we can do the deduplication before caching, but
I didn't see a big difference so I didn't want to split hairs on the
semantics.)
New timings:
baseline: 4.2s
test query:
without patch: 13.1s
0001: 11.7s
0001+0002: 10.5s
0001+0002+0003: 8.1s
New patches attached.
Regards,
Jeff Davis