From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Faster "SET search_path"
Date: Tue, 1 Aug 2023 21:52:33 -0700
Message-ID: <20230802045233.GA866741@nathanxps13> (raw)
In-Reply-To: <715f07647308639fed5389ead1e6c8de16da8eec.camel@j-davis.com>
References: <04c8592dbd694e4114a3ed87139a7a04e4363030.camel@j-davis.com>
<20230730045105.GB284134@nathanxps13>
<715f07647308639fed5389ead1e6c8de16da8eec.camel@j-davis.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, pgsql@j-davis.com
Subject: Re: Faster "SET search_path"
In-Reply-To: <20230802045233.GA866741@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox