Horton

Thread

Commits

  1. Close old gap in dependency checks for functions returning composite.

  2. Fix ruleutils issues with dropped cols in functions-returning-composite.

  1. If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered

    Timo Stolz <timo.stolz@nullachtvierzehn.de> — 2022-07-19T10:22:02Z

    Dear PostgreSQL developers,

first, I love the database! Really. For years. Like a marriage. In a 
good way. Now, on to the bug.

*
*

*Summary: If a row-level security policy contains a set returning 
function, pg_dump returns an incorrect serialization of that policy if 
the return type of the function was altered after the policy was created.*

*
*

*Affected versions: *I was able to reproduce the problem on Ubuntu Linux 
22.04 with PostgreSQL versions 13.4, 14.4, and 15beta2.


*Steps to reproduce the issue:* In the attachments, there is a minimal 
example to reproduce this bug. Please save both the attached files to a 
folder of your choice, and then run reproduce-bug.sh

If you prefer not to run the bash script, you can run the following code 
snippet instead.

createdb test
psql test < schema.sql
pg_dump test > dumped-schema.sql

createdb test_restored
psql test_restored < dumped-schema.sql

/In schema.sql, we will:/

 1. Create a table (lines 1 to 8)
 2. Create a function returning a row of this table (lines 10-22)
 3. Create two policies using this function (lines 27 to 39)
 4. Remove two columns of the table (lines 42 to 44)
 5. Dump the schema to dumped-schema.sql (using pg_dump)


*Actual and expected output:*

In the dumped schema, the policies are serialized as follows:

CREATE POLICY administrate_accounts ON public.users USING ((EXISTS ( SELECT
    FROM public.my_account() my_account(id, first_name, last_name, 
display_name, is_admin)
   WHERE my_account.is_admin)));

CREATE POLICY manage_my_account ON public.users USING ((id IN ( SELECT 
my_account.id
    FROM public.my_account() my_account(id, first_name, last_name, 
display_name, is_admin))));

Instead of this, I would expect a serialization without an aliased FROM 
clause, because that's how I wrote these policies in the first place.

CREATE POLICY administrate_accounts ON public.users USING ((EXISTS ( SELECT
    FROM public.my_account()
   WHERE my_account.is_admin)));

CREATE POLICY manage_my_account ON public.users USING ((id IN ( SELECT 
my_account.id
    FROM public.my_account() )));

As you can see from the output, the outputted table alias contains five 
columns.

my_account(id, first_name, last_name, display_name, is_admin)

This is wrong. When the policy was added, the table actually had five 
columns. But when the policy was dumped, the table had only three 
columns left over. Thus, the table alias should look like this:

my_account(id, display_name, is_admin)

In the end, I fail at restoring both the policies. In both cases, the 
*error message* is "table "my_account" has 3 columns available but 5 
columns specified"


*Further platform details:*

  * Ubuntu 22.04 LTS
  * Linux 5.15.0-41-generic x86_64
  * Ubuntu GLIBC 2.35-0ubuntu3
  * AMD Ryzen 5 5600G with Radeon Graphics
  * 32G RAM


Please ask if I can help with further details. If you open a page for 
this issue, I would be glad to know about its URL.

All the best,
Timo


-- 
Timo Stolz
Geschäftsführer
#gerneperdu


Nullachtvierzehn UG (haftungsbeschränkt)
Konstanzer Straße 15
10707 Berlin

Tel +49 (0)30 284243-87
Fax +49 (0)30 284243-88
Webwww.nullachtvierzehn.de
Mailtimo.stolz@nullachtvierzehn.de

Sitz: Berlin
Amtsgericht: Charlottenburg
Handelsregister: HRB 233776 B

Geschäftsführung: Timo Stolz

Konto bei der GLS Gemeinschaftsbank eG
IBAN: DE18 4306 0967 1256 6159 00
BIC: GENODEM1GLS

USt-IdNr.: DE346169698

  2. Re: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-20T20:54:24Z

    Timo Stolz <timo.stolz@nullachtvierzehn.de> writes:
> *Summary: If a row-level security policy contains a set returning 
> function, pg_dump returns an incorrect serialization of that policy if 
> the return type of the function was altered after the policy was created.*

This has nothing particularly to do with RLS policies; you can
reproduce the same problem with any stored query that selects from a
function-returning-composite, for instance a view.

We even have a regression test case illustrating the current behavior :-(.
I guess nobody thought hard about the implications for dump-and-restore,
but I agree that they're bad.

> Instead of this, I would expect a serialization without an aliased FROM 
> clause, because that's how I wrote these policies in the first place.

You might expect that, but you won't get it.  As noted in ruleutils.c,

     * For a relation RTE, we need only print the alias column names if any
     * are different from the underlying "real" names.  For a function RTE,
     * always emit a complete column alias list; this is to protect against
     * possible instability of the default column names (eg, from altering
     * parameter names).  For tablefunc RTEs, we never print aliases, ...

What we have to do here is to suppress the aliases for any since-dropped
columns, while keeping the live ones.  That's slightly finicky, but there
is existing code that can get the job done.  ruleutils just wasn't
considering the possibility that function RTEs might have this problem.

The larger issue that this touches on is that we don't prevent you from
dropping the composite type's column even when the query using the
dependent function has hard references to that column (e.g, it's actually
output by the view).  Maybe sometime somebody ought to work on tightening
that up.  In the meantime though, it's bad for EXPLAIN or pg_dump to fail
altogether on such cases, so I propose the behavior shown in the attached
patch.

(Even if somebody does add the necessary dependencies later, we'd still
need to cope with the situation in released branches, which might already
have broken views to cope with.)

			regards, tom lane

  3. Re: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2022-07-21T07:19:04Z

    On Wed, 20 Jul 2022 at 21:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> This has nothing particularly to do with RLS policies; you can
> reproduce the same problem with any stored query that selects from a
> function-returning-composite, for instance a view.
>

Yep, I reached the same conclusion.

> What we have to do here is to suppress the aliases for any since-dropped
> columns, while keeping the live ones.  That's slightly finicky, but there
> is existing code that can get the job done.  ruleutils just wasn't
> considering the possibility that function RTEs might have this problem.
>

Agreed. I even came up with a similar patch, but your version looks better.

> The larger issue that this touches on is that we don't prevent you from
> dropping the composite type's column even when the query using the
> dependent function has hard references to that column (e.g, it's actually
> output by the view).  Maybe sometime somebody ought to work on tightening
> that up.  In the meantime though, it's bad for EXPLAIN or pg_dump to fail
> altogether on such cases, so I propose the behavior shown in the attached
> patch.
>

+1. LGTM.

Regards,
Dean

  4. Re: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-21T17:58:44Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Wed, 20 Jul 2022 at 21:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The larger issue that this touches on is that we don't prevent you from
>> dropping the composite type's column even when the query using the
>> dependent function has hard references to that column (e.g, it's actually
>> output by the view).  Maybe sometime somebody ought to work on tightening
>> that up.  In the meantime though, it's bad for EXPLAIN or pg_dump to fail
>> altogether on such cases, so I propose the behavior shown in the attached
>> patch.

> +1. LGTM.

Pushed, thanks for reviewing!

I think I'll go take a look at the missing-dependency aspect now.
I realized from checking the commit log that we've been putting
off doing that since 2014, if not before.  Really should fix it.

			regards, tom lane

  5. Re: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-21T19:19:15Z

    I wrote:
> I think I'll go take a look at the missing-dependency aspect now.
> I realized from checking the commit log that we've been putting
> off doing that since 2014, if not before.  Really should fix it.

Here's a proposed patch for that.  I wouldn't consider pushing this
into released branches, but maybe it's not too late for v15?

			regards, tom lane

  6. Re: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2022-07-21T21:27:38Z

    On Thu, 21 Jul 2022 at 20:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > I think I'll go take a look at the missing-dependency aspect now.
> > I realized from checking the commit log that we've been putting
> > off doing that since 2014, if not before.  Really should fix it.
>
> Here's a proposed patch for that.  I wouldn't consider pushing this
> into released branches, but maybe it's not too late for v15?
>

That looks reasonable to me. It covers all the cases I could think of
to try, and I can't see any loopholes. +1 for applying it to v15.

Regards,
Dean

  7. Re: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-22T16:50:29Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Thu, 21 Jul 2022 at 20:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a proposed patch for that.  I wouldn't consider pushing this
>> into released branches, but maybe it's not too late for v15?

> That looks reasonable to me. It covers all the cases I could think of
> to try, and I can't see any loopholes. +1 for applying it to v15.

Thanks for checking it!

I had second thoughts about removing the old test cases: that would
leave us with no test coverage for the executor's defenses against
bad plans.  I'm not so foolish as to imagine we'll never introduce
another bug that would reach those defenses.  So what I did was to
adjust those cases to manually delete the new pg_depend entries,
allowing us to still test what happens without 'em.

Pushed that way.

			regards, tom lane