Message-ID: <cbb4ba82320a66c1e4a7c8fb3aa189ed9fd3908a.camel@j-davis.com>
Subject: Re: Faster "SET search_path"
From: Jeff Davis <pgsql@j-davis.com>
To: Noah Misch <noah@leadboat.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>, Isaac Morland
	 <isaac.morland@gmail.com>, pgsql-hackers@postgresql.org
Date: Mon, 08 Jul 2024 16:39:21 -0700
In-Reply-To: <20240630223047.1f.nmisch@google.com>
References: 
	<CAMsGm5fZ2xiVDLYH4-_Ld8bfe1qwY23ZLgPc7zBpH8d1WUw70A@mail.gmail.com>
	 <f555c41554c7f5ff2699c085836ea94beac09bad.camel@j-davis.com>
	 <20230802050720.GB866741@nathanxps13>
	 <CAMsGm5dt6szoTe5oAzk5nfbHHOEpMDVhLTOGWCJGCQ3Wq8+J0w@mail.gmail.com>
	 <fec7f3bea50112323d9b6e700b5e2762a14af809.camel@j-davis.com>
	 <20230807223908.GB1284531@nathanxps13>
	 <3b2d57ead15798adcbf6a53a15e35cdbadb2c147.camel@j-davis.com>
	 <5bb4d9c21cc918e5d05021bf0d5e0e7d34a0377c.camel@j-davis.com>
	 <abf4ce8804e0e05dff8c1725ae6a8ed28b7d66e0.camel@j-davis.com>
	 <695668acc40b937258985669f3d55b94cce82c3c.camel@j-davis.com>
	 <20240630223047.1f.nmisch@google.com>
Content-Type: multipart/mixed; boundary="=-EJMSLD8d+kZg4iPI9Bt2"
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/cbb4ba82320a66c1e4a7c8fb3aa189ed9fd3908a.camel%40j-davis.com>
Precedence: bulk

--=-EJMSLD8d+kZg4iPI9Bt2
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, 2024-06-30 at 15:30 -0700, Noah Misch wrote:
> You're caching the result of object_aclcheck(NamespaceRelationId,
> ...), so
> pg_auth_members changes

Thank you for the report.

Question: to check for changes to pg_auth_members, it seems like either
AUTHMEMROLEMEM or AUTHMEMMEMROLE work, and to use both would be
redundant. Am I missing something, or should I just pick one
arbitrarily (or by some convention)?

>  and pg_database changes also need to invalidate this
> cache.=C2=A0 (pg_database affects the ACL_CREATE_TEMP case in
> pg_namespace_aclmask_ext()

I am having trouble finding an actual problem with ACL_CREATE_TEMP.
search_path ACL checks are normally bypassed for the temp namespace, so
it can only happen when the actual temp namespace name (e.g.
pg_temp_NNN) is explicitly included. In that case, the mask is
ACL_USAGE, so the two branches in pg_namespace_aclmask_ext() are
equivalent, right?

This question is not terribly important for the fix, because
invalidating for each pg_database change is still necessary as you
point out in here:

>  and affects ROLE_PG_DATABASE_OWNER membership.)

Another good catch, thank you.

Patch attached. Contains a bit of cleanup and is intended for 17+18.

Regards,
	Jeff Davis


--=-EJMSLD8d+kZg4iPI9Bt2
Content-Disposition: attachment;
	filename="v1-0001-Fix-missing-invalidations-for-search_path-cache.patch"
Content-Transfer-Encoding: base64
Content-Type: text/x-patch;
	name="v1-0001-Fix-missing-invalidations-for-search_path-cache.patch";
	charset="UTF-8"

RnJvbSAxYjI0ZDA4MzAyZTI2ODM3MGViYjE0ZTczMmY0ZDVmMWYxNDA3NzI2IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBKZWZmIERhdmlzIDxqZWZmQGotZGF2aXMuY29tPgpEYXRlOiBN
b24sIDggSnVsIDIwMjQgMTU6NTI6NTAgLTA3MDAKU3ViamVjdDogW1BBVENIIHYxXSBGaXggbWlz
c2luZyBpbnZhbGlkYXRpb25zIGZvciBzZWFyY2hfcGF0aCBjYWNoZS4KClJlcG9ydGVkLWJ5OiBO
b2FoIE1pc2NoCkRpc2N1c3Npb246IGh0dHBzOi8vcG9zdGdyLmVzL20vMjAyNDA2MzAyMjMwNDcu
MWYubm1pc2NoQGdvb2dsZS5jb20KQmFja3BhdGNoLXRocm91Z2g6IDE3Ci0tLQogc3JjL2JhY2tl
bmQvY2F0YWxvZy9uYW1lc3BhY2UuYyB8IDI4ICsrKysrKysrKysrKysrKysrKysrKy0tLS0tLS0K
IDEgZmlsZSBjaGFuZ2VkLCAyMSBpbnNlcnRpb25zKCspLCA3IGRlbGV0aW9ucygtKQoKZGlmZiAt
LWdpdCBhL3NyYy9iYWNrZW5kL2NhdGFsb2cvbmFtZXNwYWNlLmMgYi9zcmMvYmFja2VuZC9jYXRh
bG9nL25hbWVzcGFjZS5jCmluZGV4IGEyNTEwY2Y4MGM2Li40M2I3MDc2OTlkNyAxMDA2NDQKLS0t
IGEvc3JjL2JhY2tlbmQvY2F0YWxvZy9uYW1lc3BhY2UuYworKysgYi9zcmMvYmFja2VuZC9jYXRh
bG9nL25hbWVzcGFjZS5jCkBAIC0yMzAsNyArMjMwLDcgQEAgc3RhdGljIHZvaWQgQWNjZXNzVGVt
cFRhYmxlTmFtZXNwYWNlKGJvb2wgZm9yY2UpOwogc3RhdGljIHZvaWQgSW5pdFRlbXBUYWJsZU5h
bWVzcGFjZSh2b2lkKTsKIHN0YXRpYyB2b2lkIFJlbW92ZVRlbXBSZWxhdGlvbnMoT2lkIHRlbXBO
YW1lc3BhY2VJZCk7CiBzdGF0aWMgdm9pZCBSZW1vdmVUZW1wUmVsYXRpb25zQ2FsbGJhY2soaW50
IGNvZGUsIERhdHVtIGFyZyk7Ci1zdGF0aWMgdm9pZCBOYW1lc3BhY2VDYWxsYmFjayhEYXR1bSBh
cmcsIGludCBjYWNoZWlkLCB1aW50MzIgaGFzaHZhbHVlKTsKK3N0YXRpYyB2b2lkIEludmFsaWRh
dGlvbkNhbGxiYWNrKERhdHVtIGFyZywgaW50IGNhY2hlaWQsIHVpbnQzMiBoYXNodmFsdWUpOwog
c3RhdGljIGJvb2wgTWF0Y2hOYW1lZENhbGwoSGVhcFR1cGxlIHByb2N0dXAsIGludCBuYXJncywg
TGlzdCAqYXJnbmFtZXMsCiAJCQkJCQkgICBib29sIGluY2x1ZGVfb3V0X2FyZ3VtZW50cywgaW50
IHByb25hcmdzLAogCQkJCQkJICAgaW50ICoqYXJnbnVtYmVycyk7CkBAIC00NzQ5LDE1ICs0NzQ5
LDI5IEBAIEluaXRpYWxpemVTZWFyY2hQYXRoKHZvaWQpCiAKIAkJLyoKIAkJICogSW4gbm9ybWFs
IG1vZGUsIGFycmFuZ2UgZm9yIGEgY2FsbGJhY2sgb24gYW55IHN5c2NhY2hlIGludmFsaWRhdGlv
bgotCQkgKiBvZiBwZ19uYW1lc3BhY2Ugb3IgcGdfYXV0aGlkIHJvd3MuIChDaGFuZ2luZyBhIHJv
bGUgbmFtZSBtYXkgYWZmZWN0Ci0JCSAqIHRoZSBtZWFuaW5nIG9mIHRoZSBzcGVjaWFsIHN0cmlu
ZyAkdXNlci4pCisJCSAqIHRoYXQgd2lsbCBhZmZlY3QgdGhlIHNlYXJjaF9wYXRoIGNhY2hlLgog
CQkgKi8KKworCQkvKiBuYW1lc3BhY2UgbmFtZSBvciBBQ0xzIG1heSBoYXZlIGNoYW5nZWQgKi8K
IAkJQ2FjaGVSZWdpc3RlclN5c2NhY2hlQ2FsbGJhY2soTkFNRVNQQUNFT0lELAotCQkJCQkJCQkJ
ICBOYW1lc3BhY2VDYWxsYmFjaywKKwkJCQkJCQkJCSAgSW52YWxpZGF0aW9uQ2FsbGJhY2ssCiAJ
CQkJCQkJCQkgIChEYXR1bSkgMCk7CisKKwkJLyogcm9sZSBuYW1lIG1heSBhZmZlY3QgdGhlIG1l
YW5pbmcgb2YgIiR1c2VyIiAqLwogCQlDYWNoZVJlZ2lzdGVyU3lzY2FjaGVDYWxsYmFjayhBVVRI
T0lELAotCQkJCQkJCQkJICBOYW1lc3BhY2VDYWxsYmFjaywKKwkJCQkJCQkJCSAgSW52YWxpZGF0
aW9uQ2FsbGJhY2ssCisJCQkJCQkJCQkgIChEYXR1bSkgMCk7CisKKwkJLyogcm9sZSBtZW1iZXJz
aGlwIG1heSBhZmZlY3QgQUNMcyAqLworCQlDYWNoZVJlZ2lzdGVyU3lzY2FjaGVDYWxsYmFjayhB
VVRITUVNUk9MRU1FTSwKKwkJCQkJCQkJCSAgSW52YWxpZGF0aW9uQ2FsbGJhY2ssCiAJCQkJCQkJ
CQkgIChEYXR1bSkgMCk7CisKKwkJLyogZGF0YWJhc2Ugb3duZXIgbWF5IGFmZmVjdCBBQ0xzICov
CisJCUNhY2hlUmVnaXN0ZXJTeXNjYWNoZUNhbGxiYWNrKERBVEFCQVNFT0lELAorCQkJCQkJCQkJ
ICBJbnZhbGlkYXRpb25DYWxsYmFjaywKKwkJCQkJCQkJCSAgKERhdHVtKSAwKTsKKwogCQkvKiBG
b3JjZSBzZWFyY2ggcGF0aCB0byBiZSByZWNvbXB1dGVkIG9uIG5leHQgdXNlICovCiAJCWJhc2VT
ZWFyY2hQYXRoVmFsaWQgPSBmYWxzZTsKIAkJc2VhcmNoUGF0aENhY2hlVmFsaWQgPSBmYWxzZTsK
QEAgLTQ3NjUsMTEgKzQ3NzksMTEgQEAgSW5pdGlhbGl6ZVNlYXJjaFBhdGgodm9pZCkKIH0KIAog
LyoKLSAqIE5hbWVzcGFjZUNhbGxiYWNrCisgKiBJbnZhbGlkYXRpb25DYWxsYmFjawogICoJCVN5
c2NhY2hlIGludmFsIGNhbGxiYWNrIGZ1bmN0aW9uCiAgKi8KIHN0YXRpYyB2b2lkCi1OYW1lc3Bh
Y2VDYWxsYmFjayhEYXR1bSBhcmcsIGludCBjYWNoZWlkLCB1aW50MzIgaGFzaHZhbHVlKQorSW52
YWxpZGF0aW9uQ2FsbGJhY2soRGF0dW0gYXJnLCBpbnQgY2FjaGVpZCwgdWludDMyIGhhc2h2YWx1
ZSkKIHsKIAkvKgogCSAqIEZvcmNlIHNlYXJjaCBwYXRoIHRvIGJlIHJlY29tcHV0ZWQgb24gbmV4
dCB1c2UsIGFsc28gaW52YWxpZGF0aW5nIHRoZQotLSAKMi4zNC4xCgo=


--=-EJMSLD8d+kZg4iPI9Bt2--