From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: Stephen Frost <sfrost@snowman.net>
Subject: Is MinMaxExpr really leakproof?
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <31041.1546194242.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 30 Dec 2018 13:24:02 -0500
Message-ID: <31042.1546194242@sss.pgh.pa.us>
Precedence: bulk

contain_leaked_vars_walker asserts the following about MinMaxExpr:

        ...
        case T_MinMaxExpr:
        ...

            /*
             * We know these node types don't contain function calls; but
             * something further down in the node tree might.
             */
            break;

Now, the idea that it "doesn't contain a function call" is nonsense,
because the node will invoke the btree comparison function for the
datatype of its arguments.  So this coding amounts to an undocumented
assumption that every non-cross-type btree comparison function is
leakproof.

A quick catalog query finds 15 counterexamples just among the
built-in datatypes:

select p.oid::regprocedure from pg_proc p, pg_amproc a, pg_opfamily f
where p.oid=3Da.amproc and f.oid=3Da.amprocfamily and opfmethod=3D403 and =
not proleakproof and amproclefttype=3Damprocrighttype and amprocnum=3D1;

 bpcharcmp(character,character)
 btarraycmp(anyarray,anyarray)
 btbpchar_pattern_cmp(character,character)
 btoidvectorcmp(oidvector,oidvector)
 btrecordcmp(record,record)
 btrecordimagecmp(record,record)
 bttext_pattern_cmp(text,text)
 bttextcmp(text,text)
 enum_cmp(anyenum,anyenum)
 jsonb_cmp(jsonb,jsonb)
 numeric_cmp(numeric,numeric)
 pg_lsn_cmp(pg_lsn,pg_lsn)
 range_cmp(anyrange,anyrange)
 tsquery_cmp(tsquery,tsquery)
 tsvector_cmp(tsvector,tsvector)

so this assumption is, on its face, wrong.

In practice it might be all right, because it's hard to see a reason why
a btree comparison function would ever throw an error except for internal
failures, which are probably outside the scope of leakproofness guarantees
anyway.  Nonetheless, if we didn't mark these functions as leakproof,
why not?

I think that we should either change contain_leaked_vars_walker to
explicitly verify leakproofness of the comparison function, or decide
that it's project policy that btree comparison functions are leakproof,
and change the markings on those (and their associated operators).

			regards, tom lane