Re: Faster "SET search_path"

On Tue, Aug 01, 2023 at 04:59:33PM -0700, Jeff Davis wrote:
> + List *pair = lfirst(lc);
> + char *name = linitial(pair);
> + char *value = lsecond(pair);

This is definitely a nitpick, but this List of Lists business feels strange
to me for some reason. There might be some code that does this, but I
typically see this done with two ѕeparate lists and forboth(). I doubt it
makes terribly much difference in the end, and I really don't have a strong
opinion either way, but it seemed like something that would inevitably come
up in this thread. Is there any particular reason you chose to do it this
way?

> 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.

That's fine by me.

> +static inline uint32
> +nspcachekey_hash(SearchPathCacheKey *key)
> +{
> + const unsigned char *bytes = (const unsigned char *)key->searchPath;
> + int blen = strlen(key->searchPath);
> +
> + return hash_bytes(bytes, blen) ^ hash_uint32(key->roleid);
> +}

Any reason not to use hash_combine() here?

> 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.)

I think you are right. This adds some complexity, but I don't have
anything else to propose at the moment.

> - /* Now safe to assign to state variables. */
> - list_free(baseSearchPath);
> - baseSearchPath = newpath;
> - baseCreationNamespace = firstNS;
> - baseTempCreationPending = temp_missing;
> }
>
> + /* Now safe to assign to state variables. */
> + list_free(baseSearchPath);
> + baseSearchPath = finalPath;
> + baseCreationNamespace = firstNS;
> + baseTempCreationPending = temp_missing;

I'm not following why this logic was moved.

> New timings:
>
> baseline: 4.2s
> test query:
> without patch: 13.1s
> 0001: 11.7s
> 0001+0002: 10.5s
> 0001+0002+0003: 8.1s

Nice. It makes sense that 0003 would provide the most benefit.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com