Date: Tue, 1 Aug 2023 21:52:33 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Faster "SET search_path"
Message-ID: <20230802045233.GA866741@nathanxps13>
References: <04c8592dbd694e4114a3ed87139a7a04e4363030.camel@j-davis.com>
 <20230730045105.GB284134@nathanxps13>
 <715f07647308639fed5389ead1e6c8de16da8eec.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <715f07647308639fed5389ead1e6c8de16da8eec.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230802045233.GA866741%40nathanxps13>
Precedence: bulk

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