Thread: psql: Add leakproof field to \dAo+ meta-command results
Hi, I would like to propose to add a new field to psql's \dAo+ meta-command to show whether the underlying function of an operator is leak-proof. This idea is inspired from [1] that claims some indexes uses non-LEAKPROOF functions under the associated operators, as a result, it can not be selected for queries with security_barrier views or row-level security policies. The original proposal was to add a query over system catalogs for looking up non-leakproof operators to the documentation, but I thought it is useful to improve \dAo results rather than putting such query to the doc. The attached patch adds the field to \dAo+ and also a description that explains the relation between indexes and security quals with referencing \dAo+ meta-command. [1] https://www.postgresql.org/message-id/raw/5af3bf0c-5e0c-4128-81dc-084c5258b1af%40code406.com Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
On 2024-07-01 15:08 +0200, Yugo NAGATA wrote: > I would like to propose to add a new field to psql's \dAo+ meta-command > to show whether the underlying function of an operator is leak-proof. +1 for making that info easily accessible. > This idea is inspired from [1] that claims some indexes uses non-LEAKPROOF > functions under the associated operators, as a result, it can not be selected > for queries with security_barrier views or row-level security policies. > The original proposal was to add a query over system catalogs for looking up > non-leakproof operators to the documentation, but I thought it is useful > to improve \dAo results rather than putting such query to the doc. > > The attached patch adds the field to \dAo+ and also a description that > explains the relation between indexes and security quals with referencing > \dAo+ meta-command. > > [1] https://www.postgresql.org/message-id/raw/5af3bf0c-5e0c-4128-81dc-084c5258b1af%40code406.com \dAo+ output looks good. But this patch fails regression tests in src/test/regress/sql/psql.sql (\dAo+ btree float_ops) because of the new leak-proof column. I think this could even be changed to "\dAo+ btree array_ops|float_ops" to also cover operators that are not leak-proof. +<para> + For example, an index scan can not be selected for queries with I check the docs and "cannot" is more commonly used than "can not". + <literal>security_barrier</literal> views or row-level security policies if an + operator used in the <literal>WHERE</literal> clause is associated with the + operator family of the index, but its underlying function is not marked + <literal>LEAKPROOF</literal>. The <xref linkend="app-psql"/> program's + <command>\dAo+</command> meta-command is useful for listing the operators + with associated operator families and whether it is leak-proof. +</para> I think the last sentence can be improved. How about: "Use psql's \dAo+ command to list operator families and tell which of their operators are marked as leak-proof."? Should something similar be added to [1] which also talks about leak-proof operators? The rest is just formatting nitpicks: + ", ofs.opfname AS \"%s\"\n," The trailing comma should come before the newline. + " CASE\n" + " WHEN p.proleakproof THEN '%s'\n" + " ELSE '%s'\n" + " END AS \"%s\"\n", WHEN/ELSE/END should be intended with one additional space to be consistent with the other CASE expressions in this query. [1] https://www.postgresql.org/docs/devel/planner-stats-security.html -- Erik
Hi, On Tue, 30 Jul 2024 01:36:55 +0200 Erik Wienhold <ewie@ewie.name> wrote: > On 2024-07-01 15:08 +0200, Yugo NAGATA wrote: > > I would like to propose to add a new field to psql's \dAo+ meta-command > > to show whether the underlying function of an operator is leak-proof. > > +1 for making that info easily accessible. > > > This idea is inspired from [1] that claims some indexes uses non-LEAKPROOF > > functions under the associated operators, as a result, it can not be selected > > for queries with security_barrier views or row-level security policies. > > The original proposal was to add a query over system catalogs for looking up > > non-leakproof operators to the documentation, but I thought it is useful > > to improve \dAo results rather than putting such query to the doc. > > > > The attached patch adds the field to \dAo+ and also a description that > > explains the relation between indexes and security quals with referencing > > \dAo+ meta-command. > > > > [1] https://www.postgresql.org/message-id/raw/5af3bf0c-5e0c-4128-81dc-084c5258b1af%40code406.com > > \dAo+ output looks good. Thank you for looking into this. I attached a patch updated with your suggestions. > > But this patch fails regression tests in src/test/regress/sql/psql.sql > (\dAo+ btree float_ops) because of the new leak-proof column. I think > this could even be changed to "\dAo+ btree array_ops|float_ops" to also > cover operators that are not leak-proof. Thank you for pointing out this. I fixed it with you suggestion to cover non leak-proof operators, too. > +<para> > + For example, an index scan can not be selected for queries with > > I check the docs and "cannot" is more commonly used than "can not". Fixed. > > + <literal>security_barrier</literal> views or row-level security policies if an > + operator used in the <literal>WHERE</literal> clause is associated with the > + operator family of the index, but its underlying function is not marked > + <literal>LEAKPROOF</literal>. The <xref linkend="app-psql"/> program's > + <command>\dAo+</command> meta-command is useful for listing the operators > + with associated operator families and whether it is leak-proof. > +</para> > > I think the last sentence can be improved. How about: "Use psql's \dAo+ > command to list operator families and tell which of their operators are > marked as leak-proof."? Should something similar be added to [1] which > also talks about leak-proof operators? I agree, so I fixed the sentence as your suggestion and also add the same description to the planner-stats-security doc. > The rest is just formatting nitpicks: > > + ", ofs.opfname AS \"%s\"\n," > > The trailing comma should come before the newline. > > + " CASE\n" > + " WHEN p.proleakproof THEN '%s'\n" > + " ELSE '%s'\n" > + " END AS \"%s\"\n", > > WHEN/ELSE/END should be intended with one additional space to be > consistent with the other CASE expressions in this query. Fixed both. Regards, Yugo Nagata > > [1] https://www.postgresql.org/docs/devel/planner-stats-security.html > > -- > Erik -- Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
On 2024-07-30 08:30 +0200, Yugo NAGATA wrote: > On Tue, 30 Jul 2024 01:36:55 +0200 > Erik Wienhold <ewie@ewie.name> wrote: > > > On 2024-07-01 15:08 +0200, Yugo NAGATA wrote: > > > I would like to propose to add a new field to psql's \dAo+ meta-command > > > to show whether the underlying function of an operator is leak-proof. > > > > +1 for making that info easily accessible. > > > > > This idea is inspired from [1] that claims some indexes uses non-LEAKPROOF > > > functions under the associated operators, as a result, it can not be selected > > > for queries with security_barrier views or row-level security policies. > > > The original proposal was to add a query over system catalogs for looking up > > > non-leakproof operators to the documentation, but I thought it is useful > > > to improve \dAo results rather than putting such query to the doc. > > > > > > The attached patch adds the field to \dAo+ and also a description that > > > explains the relation between indexes and security quals with referencing > > > \dAo+ meta-command. > > > > > > [1] https://www.postgresql.org/message-id/raw/5af3bf0c-5e0c-4128-81dc-084c5258b1af%40code406.com > > > > \dAo+ output looks good. > > Thank you for looking into this. > I attached a patch updated with your suggestions. LGTM, thanks. > > > > But this patch fails regression tests in src/test/regress/sql/psql.sql > > (\dAo+ btree float_ops) because of the new leak-proof column. I think > > this could even be changed to "\dAo+ btree array_ops|float_ops" to also > > cover operators that are not leak-proof. > > Thank you for pointing out this. I fixed it with you suggestion to cover > non leak-proof operators, too. > > > +<para> > > + For example, an index scan can not be selected for queries with > > > > I check the docs and "cannot" is more commonly used than "can not". > > Fixed. > > > > > + <literal>security_barrier</literal> views or row-level security policies if an > > + operator used in the <literal>WHERE</literal> clause is associated with the > > + operator family of the index, but its underlying function is not marked > > + <literal>LEAKPROOF</literal>. The <xref linkend="app-psql"/> program's > > + <command>\dAo+</command> meta-command is useful for listing the operators > > + with associated operator families and whether it is leak-proof. > > +</para> > > > > I think the last sentence can be improved. How about: "Use psql's \dAo+ > > command to list operator families and tell which of their operators are > > marked as leak-proof."? Should something similar be added to [1] which > > also talks about leak-proof operators? > > I agree, so I fixed the sentence as your suggestion and also add the > same description to the planner-stats-security doc. > > > The rest is just formatting nitpicks: > > > > + ", ofs.opfname AS \"%s\"\n," > > > > The trailing comma should come before the newline. > > > > + " CASE\n" > > + " WHEN p.proleakproof THEN '%s'\n" > > + " ELSE '%s'\n" > > + " END AS \"%s\"\n", > > > > WHEN/ELSE/END should be intended with one additional space to be > > consistent with the other CASE expressions in this query. > > Fixed both. > > Regards, > Yugo Nagata > > > > > [1] https://www.postgresql.org/docs/devel/planner-stats-security.html > > > > -- > > Erik > > > -- > Yugo NAGATA <nagata@sraoss.co.jp> -- Erik
> On 2024-07-01 15:08 +0200, Yugo NAGATA wrote: > I would like to propose to add a new field to psql's \dAo+ meta-command > to show whether the underlying function of an operator is leak-proof. > I agree that this is useful information to have, but why add it to \dAo+ instead of \do+? Taking the example from the original thread, when writing a query containing 'tsvector @@ tsquery', it's much more obvious to use "\do+ @@" to check if it's leakproof, rather than "\dAo+ gin". Perhaps it would be useful to have this in \df+ output as well. I notice that this patch spells "leakproof" with a hyphen. IMO leakproof should not have a hyphen -- at least, that's how I naturally spell it, and I think that's more common, and it matches the SQL syntax. We haven't been consistent about that in the docs and code comments so far though, so I think we should make a decision, and then standardise on whatever people decide. Regards, Dean
On Mon, 4 Nov 2024 11:00:41 +0000 Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On 2024-07-01 15:08 +0200, Yugo NAGATA wrote: > > I would like to propose to add a new field to psql's \dAo+ meta-command > > to show whether the underlying function of an operator is leak-proof. > > > > I agree that this is useful information to have, but why add it to > \dAo+ instead of \do+? Taking the example from the original thread, > when writing a query containing 'tsvector @@ tsquery', it's much more > obvious to use "\do+ @@" to check if it's leakproof, rather than > "\dAo+ gin". I added it to \dAo+ since the initial motivation was that it enables to check whether we can use an index scan for scanning a table which has RLS policy when the condition contains a certain operator. However, as you suggested, adding it to \do+ seems enough to know conditions using specified operators can use indexes. I'll fixed the patch to add leakproof info to \do+ results, but is it worth leaving this info in \dAo+ results, too? > Perhaps it would be useful to have this in \df+ output as well. Agreed. I'll add the info to \df+, too. > I notice that this patch spells "leakproof" with a hyphen. IMO > leakproof should not have a hyphen -- at least, that's how I naturally > spell it, and I think that's more common, and it matches the SQL > syntax. OK, I'll fix it to use "leakproof" without a hyphen. > We haven't been consistent about that in the docs and code comments so > far though, so I think we should make a decision, and then standardise > on whatever people decide. I am not a native English speaker, but if this is natural spelling in English, I wonder we can replace all of them to leakproof without a hyphen. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
On Fri, 15 Nov 2024 at 09:55, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > I'll fixed the patch to add leakproof info to \do+ results, but is it worth > leaving this info in \dAo+ results, too? > I suppose that might still be useful in some contexts. Looking through the complete list of psql meta-commands, "leakproof" could plausibly be added to the output of each of the following: \dAo+ \dC+ \df+ \do+ > > I notice that this patch spells "leakproof" with a hyphen. IMO > > leakproof should not have a hyphen -- at least, that's how I naturally > > spell it, and I think that's more common, and it matches the SQL > > syntax. > > OK, I'll fix it to use "leakproof" without a hyphen. > > > We haven't been consistent about that in the docs and code comments so > > far though, so I think we should make a decision, and then standardise > > on whatever people decide. > > I am not a native English speaker, but if this is natural spelling in > English, I wonder we can replace all of them to leakproof without a hyphen. > Yes, I think we should do that (in a separate patch). Regards, Dean