From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
cc: pgsql-hackers@lists.postgresql.org, alex work <alexwork033@gmail.com>,
        Robert Haas <robertmhaas@gmail.com>
Subject: Re: Slow GRANT ROLE on PostgreSQL 16 with thousands of ROLEs
In-reply-to: <20240322015251.GA2193900@nathanxps13>
References: <CAGvXd3OSMbJQwOSc-Tq-Ro1CAz=vggErdSG7pv2s6vmmTOLJSg@mail.gmail.com> <341186.1711037256@sss.pgh.pa.us> <619340.1711050134@sss.pgh.pa.us> <642442.1711053105@sss.pgh.pa.us> <20240321204012.GA1992418@nathanxps13> <20240322005139.GA2060816@nathanxps13> <806361.1711069194@sss.pgh.pa.us> <20240322010332.GA2109205@nathanxps13> <20240322015251.GA2193900@nathanxps13>
Comments: In-reply-to Nathan Bossart <nathandbossart@gmail.com>
	message dated "Thu, 21 Mar 2024 20:52:51 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <813832.1711073239.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 21 Mar 2024 22:07:19 -0400
Message-ID: <813833.1711073239@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/813833.1711073239%40sss.pgh.pa.us>
Precedence: bulk

Nathan Bossart <nathandbossart@gmail.com> writes:
> The Bloom filter appears to help a bit, although it regresses the
> create-roles.sql portion of the test.  I'm assuming that's thanks to all
> the extra pallocs and pfrees, which are probably avoidable if we store t=
he
> filter in a long-lived context and just clear it at the beginning of eac=
h
> call to roles_is_member_of().

The zero-fill to reinitialize the filter probably costs a good deal
all by itself, considering we're talking about at least a megabyte.
Maybe a better idea is to not enable the filter till we're dealing
with at least 1000 or so entries in roles_list, though obviously that
will complicate the logic.

+            if (bloom_lacks_element(bf, (unsigned char *) &otherid, sizeo=
f(Oid)))
+                roles_list =3D lappend_oid(roles_list, otherid);
+            else
+                roles_list =3D list_append_unique_oid(roles_list, otherid=
);
+            bloom_add_element(bf, (unsigned char *) &otherid, sizeof(Oid)=
);

Hmm, I was imagining something more like

        if (bloom_lacks_element(bf, (unsigned char *) &otherid, sizeof(Oid=
)) ||
            !list_member_oid(roles_list, otherid))
        {
            roles_list =3D lappend_oid(roles_list, otherid);
            bloom_add_element(bf, (unsigned char *) &otherid, sizeof(Oid))=
;
        }

to avoid duplicate bloom_add_element calls.  Probably wouldn't move
the needle much in this specific test case, but this formulation
would simplify letting the filter kick in later.  Very roughly,

        if ((bf && bloom_lacks_element(bf, (unsigned char *) &otherid, siz=
eof(Oid))) ||
            !list_member_oid(roles_list, otherid))
        {
            if (bf =3D=3D NULL && list_length(roles_list) > 1000)
            {
                ... create bf and populate with existing list entries
            }
            roles_list =3D lappend_oid(roles_list, otherid);
            if (bf)
                bloom_add_element(bf, (unsigned char *) &otherid, sizeof(O=
id));
        }


			regards, tom lane