Re: 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] /message-id/raw/5af3bf0c-5e0c-4128-81dc-084c5258b1af%40code406.com

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>

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] /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] /docs/devel/planner-stats-security.html

--
Erik

Hi,

On Tue, 30 Jul 2024 01:36:55 +0200
Erik Wienhold <ewie(at)ewie(dot)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] /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] /docs/devel/planner-stats-security.html
>
> --
> Erik

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>

On 2024-07-30 08:30 +0200, Yugo NAGATA wrote:
> On Tue, 30 Jul 2024 01:36:55 +0200
> Erik Wienhold <ewie(at)ewie(dot)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] /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] /docs/devel/planner-stats-security.html
> >
> > --
> > Erik
>
>
> --
> Yugo NAGATA <nagata(at)sraoss(dot)co(dot)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(dot)a(dot)rasheed(at)gmail(dot)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(at)sraoss(dot)co(dot)jp>

On Fri, 15 Nov 2024 at 09:55, Yugo Nagata <nagata(at)sraoss(dot)co(dot)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

On Fri, Nov 15, 2024 at 05:26:08PM +0000, Dean Rasheed wrote:
> Yes, I think we should do that (in a separate patch).

$ git grep "leakproof" | wc -l
544
$ git grep "leak-proof" | wc -l
8

So there's a clear winner here.
--
Michael

On Fri, 15 Nov 2024 17:26:08 +0000
Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> On Fri, 15 Nov 2024 at 09:55, Yugo Nagata <nagata(at)sraoss(dot)co(dot)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've attached a updated patch (v3-0001) that include changes on all
of these meta-commands.

>
> > > 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).

And, the patch v3-0002 is for that.

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>

On Wed, 4 Dec 2024 at 11:21, Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> > 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've attached a updated patch (v3-0001) that include changes on all
> of these meta-commands.
>

Nice. I think this is very useful.

I spotted one issue, which can be seen by compiling with --enable-nls
and --enable-cassert. In that case \dC+ fails with an assertion error:

\dC+ json
psql: print.c:3564: printQuery: Assertion `opt->translate_columns ==
((void *)0) || opt->n_translate_columns >= cont.ncolumns' failed.
Aborted (core dumped)

This is because translate_columns[] in listCasts() needs to be updated.

Similarly, in describeFunctions(), translate_columns_pre_96[] needs to
be updated to support connecting to pre-9.6 servers.

The translate_columns entries for this new column should be true, so
that the "yes"/"no" gets appropriately translated. That means that
describeOperators() will need a similar translate_columns array.

Regards,
Dean

On Fri, 10 Jan 2025 11:31:39 +0000
Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> On Wed, 4 Dec 2024 at 11:21, Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >
> > > 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've attached a updated patch (v3-0001) that include changes on all
> > of these meta-commands.
> >
>
> Nice. I think this is very useful.
>
> I spotted one issue, which can be seen by compiling with --enable-nls
> and --enable-cassert. In that case \dC+ fails with an assertion error:
>
> \dC+ json
> psql: print.c:3564: printQuery: Assertion `opt->translate_columns ==
> ((void *)0) || opt->n_translate_columns >= cont.ncolumns' failed.
> Aborted (core dumped)
>
> This is because translate_columns[] in listCasts() needs to be updated.
>
> Similarly, in describeFunctions(), translate_columns_pre_96[] needs to
> be updated to support connecting to pre-9.6 servers.
>
> The translate_columns entries for this new column should be true, so
> that the "yes"/"no" gets appropriately translated. That means that
> describeOperators() will need a similar translate_columns array.

Thank you for pointing out this.
I've attached a updated patch v4 that includes fixes on translate_columns[]
and ranslate_columns_pre_96[].

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>

On Tue, 14 Jan 2025 at 08:44, Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> I've attached a updated patch v4 that includes fixes on translate_columns[]
> and ranslate_columns_pre_96[].
>

This looked good to me, so I've pushed both patches.

I changed the column name to "Leakproof?" with a question mark,
because all other boolean columns in psql meta-commands end with a
question mark.

Regards,
Dean

On Tue, 14 Jan 2025 13:58:18 +0000
Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> On Tue, 14 Jan 2025 at 08:44, Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >
> > I've attached a updated patch v4 that includes fixes on translate_columns[]
> > and ranslate_columns_pre_96[].
> >
>
> This looked good to me, so I've pushed both patches.
>
> I changed the column name to "Leakproof?" with a question mark,
> because all other boolean columns in psql meta-commands end with a
> question mark.

Thank you!

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>