Re: Many of psql's describe functions bloat cache / waste mem

Hi Hackers,

On 24.07.26 05:17, xiaoyu liu wrote:
[...]

> If anyone has already started reviewing it, or sees an issue with the
> current approach, please let me know.

Thanks for the patch. I tested it and it applied to the current master
branch (a1bb92fb), and check-world passes. I checked the size of the
CacheMemoryContext before and after running \df:

master(at)a1bb92fb: 8,705,984 B
patch applied: 1,264,384 B

So the patch clearly reduces the size of the CacheMemoryContext
populated by this query. I also tested the query plan shape. As
mentioned in the comment of the patch, the OID lookup subquery becomes
an InitPlan:

QUERY PLAN

---------------------------------------------------------------------------------------------------------
Sort (cost=268.31..268.32 rows=1 width=224)
Sort Key: n.nspname, p.proname, (pg_get_function_arguments(p.oid))
InitPlan array_1
-> Seq Scan on pg_namespace (cost=0.00..1.06 rows=2 width=4)
Filter: (nspname = ANY
('{pg_catalog,information_schema}'::name[]))
-> Nested Loop Left Join (cost=0.00..267.24 rows=1 width=224)
Join Filter: (n.oid = p.pronamespace)
-> Seq Scan on pg_proc p (cost=0.00..266.11 rows=1 width=73)
Filter: ((pronamespace <> ALL ((InitPlan array_1).col1))
AND pg_function_is_visible(oid))
-> Seq Scan on pg_namespace n (cost=0.00..1.05 rows=5 width=68)
(10 rows)

Now a 'Nested Loop Left Join' is used. As Andres pointed out, this LEFT
JOIN could be changed to a JOIN.

Tests
=====

I think I found an issue in the test. According to the test description,
it ensures "filters system functions before testing visibility". To do
so, the filter must run first, then pg_function_is_visible(). This
happens currently as desired in the query plan.

Filter: ((pronamespace <> ALL ((InitPlan array_1).col1)) AND
pg_function_is_visible(oid))

However, PostgreSQL could reorder both conditions (they are ordered that
way because of their costs). But the costs could change for whatever
reason. For example, if you run "ALTER FUNCTION
pg_catalog.pg_function_is_visible(oid) COST 1;", the conditions in the
query filter flip to:

Filter: (pg_function_is_visible(oid) AND (pronamespace <> ALL ((InitPlan
array_1).col1)))

In this case, pg_function_is_visible() runs for every row found in the
catalog. I verified this using 'funccount' and running \df in parallel
(in my database I have 6 user functions and 3455 catalog functions defined):

# Filter first and then pg_function_is_visible(oid)

$ sudo funccount-bpfcc "$(pg_config
--bindir)/postgres:pg_function_is_visible"
[...]
FUNC COUNT
pg_function_is_visible 6
Detaching...

# pg_function_is_visible(oid) first and then the filter

$ sudo funccount-bpfcc "$(pg_config
--bindir)/postgres:pg_function_is_visible"
[...]
FUNC COUNT
pg_function_is_visible 3461
Detaching...

The regex only checks the SQL text that psql generates, not the
resulting query plan. That text is identical in both cases, so the test
cannot detect this regression at all: with the procost changed as above,
pg_function_is_visible() is called for every row in pg_proc and
001_basic.pl still passes.

That said, I am not sure if we need such kind of test in the end. For
example, 112e40b867b reordered clauses for plan reasons in describe.c
and went in without tests.

Other \d commands
=================

As Andres outlined, there are more \d commands affected by this problem
and only \df is changed by the current version of this patch. For
example, \do uses the old shape of the query plan where
'pg_operator_is_visible' is called for every operator.

The query plan for \do:

QUERY PLAN

-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Sort (cost=133.01..133.41 rows=161 width=256)
Sort Key: n.nspname, o.oprname, (CASE WHEN (o.oprkind = 'l'::"char")
THEN NULL::text ELSE format_type(o.oprleft, NULL::integer) END), (CASE
WHEN (o.oprkind = 'r'::"char") THEN NULL::text ELSE
format_type(o.oprright, NULL::integer) END)
-> Hash Join (cost=1.11..127.10 rows=161 width=256)
Hash Cond: (o.oprnamespace = n.oid)
-> Seq Scan on pg_operator o (cost=0.00..42.18 rows=268 width=89)
Filter: pg_operator_is_visible(oid)
-> Hash (cost=1.07..1.07 rows=3 width=68)
-> Seq Scan on pg_namespace n (cost=0.00..1.07 rows=3
width=68)
Filter: ((nspname <> 'pg_catalog'::name) AND
(nspname <> 'information_schema'::name))
(9 rows)

What do you think about introducing a helper function like
'appendSystemSchemaFilter()' that adds the filter condition as proposed
in the patch, and calling it from the other places in describe.c that
exclude the system schemas?

Best regards
Jan

--
Jan Nidzwetzki
PlanetScale Postgres Core Team