Thread: BUG #13891: Deparsed arbiter WHERE clauses cannot be parsed by Postgres

The following bug has been logged on the website:

Bug reference:      13891
Logged by:          Onder Kalaci
Email address:      onderkalaci@gmail.com
PostgreSQL version: 9.5.0
Operating system:   MacOs
Description:

Deparsed arbiter WHERE clauses cannot be parsed by Postgres. Please follow
the steps below:

I create two tables:
CREATE TABLE test_1 (a int, b int);
CREATE TABLE test_2 (a int UNIQUE, b int);

Create a rule:
CREATE RULE r3 AS ON INSERT TO test_1
    DO INSTEAD
    INSERT INTO test_2 VALUES (1,1) ON CONFLICT(a) WHERE a > 100
        DO UPDATE set b = test_2.b+1;

Then, SELECT * FROM pg_rules; I get the following:

CREATE RULE r3 AS ON INSERT TO test_1
    DO INSTEAD
    INSERT INTO test_2 (a, b) VALUES (1, 1) ON CONFLICT(a) WHERE (test_2.a >
100)
    DO UPDATE SET b = (test_2.b + 1);

Cutting the query part alone:

INSERT INTO test_2 (a, b) VALUES (1, 1) ON CONFLICT(a) WHERE (test_2.a >
100)
    DO UPDATE SET b = (test_2.b + 1);

This query errors out saying:
ERROR:  invalid reference to FROM-clause entry for table "test_2"
LINE 1: ...test_2 (a, b) VALUES (1, 1) ON CONFLICT(a) WHERE (test_2.a >...
                                                             ^
HINT:  There is an entry for table "test_2", but it cannot be referenced
from this part of the query.

I guess the problem is on this line (ruletils@5533 on Postgresql 9.5.0
source)
https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/ruleutils.c#L5533

I think, postgres should set varprefix to false before calling
get_rule_expr().

Thanks,
Onder

On Tue, Jan 26, 2016 at 4:34 AM,  <onderkalaci@gmail.com> wrote:
>
> I guess the problem is on this line (ruletils@5533 on Postgresql 9.5.0
> source)
> https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/ruleutils.c#L5533
>
> I think, postgres should set varprefix to false before calling
> get_rule_expr().

I believe that your analysis is correct. I'll need to look into it in
more detail, but I'm about to get on a transatlantic flight.

I'm curious: How did you find this bug? Did you just stumble upon it?

I think that the deeper problem here may be that parse analysis of the
inference WHERE clause works by reusing infrastructure from CREATE
INDEX. We should consider that there may be further consequences to
that (although there may well not be).

Does the rewritten query actually error? In other words, is your
complaint strictly that deparsing is broken?

--
Peter Geoghegan

On Wed, Jan 27, 2016 at 5:42 AM, Önder Kalacı <onderkalaci@gmail.com> wrote:
> We use postgres' ruleutils.c to build SQL strings. I realized this while I
> was playing with UPSERTs on 9.5.

Here is a bug fix patch.

InferenceElem already had appropriate handling for this case.
Unfortunately, that did no help with the arbiter WHERE clause, since
it does not use InferenceElem. I haven't moved the existing handling;
I feel it makes sense to do this separately for each case.

Thanks for the report!

--
Peter Geoghegan

On 2016-02-04 04:02:04 -0800, Peter Geoghegan wrote:
> --- a/src/test/regress/expected/rules.out
> +++ b/src/test/regress/expected/rules.out
> @@ -2846,7 +2846,7 @@ SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename;
>   CREATE RULE hat_nosert AS                                                                  +
>       ON INSERT TO hats DO INSTEAD  INSERT INTO hat_data (hat_name, hat_color)               +
>     VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name COLLATE "C" bpchar_pattern_ops)+
> -   WHERE (hat_data.hat_color = 'green'::bpchar) DO NOTHING                                  +
> +   WHERE (hat_color = 'green'::bpchar) DO NOTHING                                           +
>     RETURNING hat_data.hat_name,                                                             +
>       hat_data.hat_color;
>  (1 row)
> @@ -2871,7 +2871,7 @@ SELECT tablename, rulename, definition FROM pg_rules
>   hats      | hat_nosert | CREATE RULE hat_nosert AS
+
>             |            |     ON INSERT TO hats DO INSTEAD  INSERT INTO hat_data (hat_name, hat_color)
+
>             |            |   VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name COLLATE "C"
bpchar_pattern_ops)+
> -           |            |   WHERE (hat_data.hat_color = 'green'::bpchar) DO NOTHING
+
> +           |            |   WHERE (hat_color = 'green'::bpchar) DO NOTHING
+
>             |            |   RETURNING hat_data.hat_name,
+
>             |            |     hat_data.hat_color;
>  (1 row)

Can you add code to execute the resulting statements? Because obviously
just seeing the output ain't enough. Seems like an easily repeatable
error, and the regression test scase ought to be pretty easy with a DO
and a loop.

Andres

On Thu, Feb 4, 2016 at 4:11 AM, Andres Freund <andres@anarazel.de> wrote:
> Can you add code to execute the resulting statements? Because obviously
> just seeing the output ain't enough. Seems like an easily repeatable
> error, and the regression test scase ought to be pretty easy with a DO
> and a loop.

What do you mean?

The stored rules whose output was modified as part of the fix are
actually executed already -- no change there. =C3=96nder's complaint was
that deparsing doesn't work. It was not that stored rules were ever
independently broken.

--=20
Peter Geoghegan

On Thu, Feb 4, 2016 at 6:16 AM, Peter Geoghegan <pg@heroku.com> wrote:
> The stored rules whose output was modified as part of the fix are
> actually executed already -- no change there.

By which I mean: Real INSERT statements are rewritten and executed,
with no obvious trouble.

--
Peter Geoghegan

On February 4, 2016 5:16:16 PM GMT+03:00, Peter Geoghegan <pg@heroku.com> wrote:
>On Thu, Feb 4, 2016 at 4:11 AM, Andres Freund <andres@anarazel.de>
>wrote:
>> Can you add code to execute the resulting statements? Because
>obviously
>> just seeing the output ain't enough. Seems like an easily repeatable
>> error, and the regression test scase ought to be pretty easy with a
>DO
>> and a loop.
>
>What do you mean?
>
>The stored rules whose output was modified as part of the fix are
>actually executed already -- no change there. Önder's complaint was
>that deparsing doesn't work. It was not that stored rules were ever
>independently broken.

I want the deparse output to be executed.

Andres

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

On Thu, Feb 4, 2016 at 7:15 AM, Andres Freund <andres@anarazel.de> wrote:
>>The stored rules whose output was modified as part of the fix are
>>actually executed already -- no change there. =C3=96nder's complaint was
>>that deparsing doesn't work. It was not that stored rules were ever
>>independently broken.
>
> I want the deparse output to be executed.

That seems like something requiring an additional infrastructure. It
would be somewhat useful to smoke test lots of things like this. What
exactly do you have in mind?


--=20
Peter Geoghegan

On 2016-02-04 12:43:15 -0800, Peter Geoghegan wrote:
> On Thu, Feb 4, 2016 at 7:15 AM, Andres Freund <andres@anarazel.de> wrote:
> >>The stored rules whose output was modified as part of the fix are
> >>actually executed already -- no change there. Önder's complaint was
> >>that deparsing doesn't work. It was not that stored rules were ever
> >>independently broken.
> >
> > I want the deparse output to be executed.
>
> That seems like something requiring an additional infrastructure. It
> would be somewhat useful to smoke test lots of things like this. What
> exactly do you have in mind?

Seems fairly simple to write a DO statement that does something like
v_sql = pg_get_viewdef(viewname);
EXECUTE 'DROP VIEW '||viewname::regclass;
EXECUTE v_sql;
in a query over pg_views.

On Thu, Feb 4, 2016 at 12:49 PM, Andres Freund <andres@anarazel.de> wrote:
> Seems fairly simple to write a DO statement that does something like
> v_sql = pg_get_viewdef(viewname);
> EXECUTE 'DROP VIEW '||viewname::regclass;
> EXECUTE v_sql;
> in a query over pg_views.

Sorry, I still don't get it. How does that help with ON CONFLICT
arbiter WHERE clause deparsing?


--
Peter Geoghegan

On 2016-02-04 13:09:43 -0800, Peter Geoghegan wrote:
> On Thu, Feb 4, 2016 at 12:49 PM, Andres Freund <andres@anarazel.de> wrote:
> > Seems fairly simple to write a DO statement that does something like
> > v_sql = pg_get_viewdef(viewname);
> > EXECUTE 'DROP VIEW '||viewname::regclass;
> > EXECUTE v_sql;
> > in a query over pg_views.
>
> Sorry, I still don't get it. How does that help with ON CONFLICT
> arbiter WHERE clause deparsing?

The bug this thread is about was actually visible in regression test
output, as the wrong output of pg_rules output. As were a number of
previous bugs. So it seems time to actually make sure that the output of
the rules we generate to test deparsing actually do something remotely
sane, by executing the deparsed sql. Obviously human inspection is not
sufficient.  You can do so by executing deparsed output of a rule or a
view (both should be doable afacis).

Andres Freund <andres@anarazel.de> writes:
> On 2016-02-04 13:09:43 -0800, Peter Geoghegan wrote:
>> Sorry, I still don't get it. How does that help with ON CONFLICT
>> arbiter WHERE clause deparsing?

> The bug this thread is about was actually visible in regression test
> output, as the wrong output of pg_rules output. As were a number of
> previous bugs. So it seems time to actually make sure that the output of
> the rules we generate to test deparsing actually do something remotely
> sane, by executing the deparsed sql. Obviously human inspection is not
> sufficient.  You can do so by executing deparsed output of a rule or a
> view (both should be doable afacis).

That's all very well, but we have a release to get out tomorrow, so
I went ahead and pushed Peter's patch (with some cosmetic fiddling).

I think actually Andres' proposal wouldn't be all that helpful, because
it would only detect deparse problems for constructs that happen to
appear in views/rules that exist at the instant the test runs.

What I'd be inclined to think about is something comparable to the
COPY_PARSE_PLAN_TREES #define, that is an option to make postgres.c
pass every non-utility statement through deparse and reparse and then
compare the resulting parse trees.  It'd be a bit slow but it would
cover every type of syntax exercised anywhere in the regression tests.

            regards, tom lane

On February 7, 2016 9:02:11 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andres Freund <andres@anarazel.de> writes:

>I think actually Andres' proposal wouldn't be all that helpful, because
>it would only detect deparse problems for constructs that happen to
>appear in views/rules that exist at the instant the test runs.

That's be fine for this purpose. This specific line had two hard to spot roots already...

---
Please excuse brevity and formatting - I am writing this on my mobile phone.