Re: RULE regression test fragility?

While reviewing the Network Stats Traffic patch I discovered the current
regression test for rules depends on the system view definitions not
changing:

--
-- Check that ruleutils are working
--
SELECT viewname, definition FROM pg_views WHERE schemaname <>
'information_schema' ORDER BY viewname;

In this particular case new fields have been added to the view, breaking
this apparently unrelated test. Is checking the definition of all views
necessary for this test? Would it possibly be better to create a temporary
view for this check, or is something else going on here?

​​
__________________________________________________________________________________
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
Mike(dot)Blackwell(at)rrd(dot)com
http://www.rrdonnelley.com

<http://www.rrdonnelley.com/>
* <Mike(dot)Blackwell(at)rrd(dot)com>*

Mike Blackwell <mike(dot)blackwell(at)rrd(dot)com> writes:
> While reviewing the Network Stats Traffic patch I discovered the current
> regression test for rules depends on the system view definitions not
> changing:

Yes, this is standard. We just update the expected output anytime
somebody changes a system view.

(Now, if the submitter failed to notice that his patch broke the
regression tests, that's grounds to wonder how much he tested it.
But it's not unexpected for that test's output to change.)

> [ Is it really a good idea for the regression tests to do that? ]

I tend to think so, as it seems like a good stress test for the
rule-dumping machinery.

regards, tom lane

On 2013-10-23 20:50:30 -0400, Tom Lane wrote:
> Mike Blackwell <mike(dot)blackwell(at)rrd(dot)com> writes:
> > While reviewing the Network Stats Traffic patch I discovered the current
> > regression test for rules depends on the system view definitions not
> > changing:
>
> Yes, this is standard. We just update the expected output anytime
> somebody changes a system view.

FWIW, I've repeatedly now thought that it'd make maintaining/updating
patches easier if we switched that query into unaligned tuple only (\a
\t) mode. That would remove the frequent conflicts on the row count and
widespread changes due to changed alignment.
Alternatively we could just wrap the query in \copy ... CSV.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> FWIW, I've repeatedly now thought that it'd make maintaining/updating
> patches easier if we switched that query into unaligned tuple only (\a
> \t) mode. That would remove the frequent conflicts on the row count and
> widespread changes due to changed alignment.
> Alternatively we could just wrap the query in \copy ... CSV.

Hm ... yeah, it would be a good thing if changes in one view didn't so
frequently have ripple effects to the whole output. Not sure which
format is best for that though.

regards, tom lane

On 2013-10-24 09:22:52 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > FWIW, I've repeatedly now thought that it'd make maintaining/updating
> > patches easier if we switched that query into unaligned tuple only (\a
> > \t) mode. That would remove the frequent conflicts on the row count and
> > widespread changes due to changed alignment.
> > Alternatively we could just wrap the query in \copy ... CSV.
>
> Hm ... yeah, it would be a good thing if changes in one view didn't so
> frequently have ripple effects to the whole output. Not sure which
> format is best for that though.

Something like the attached maybe?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund wrote:
> On 2013-10-24 09:22:52 -0400, Tom Lane wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > FWIW, I've repeatedly now thought that it'd make maintaining/updating
> > > patches easier if we switched that query into unaligned tuple only (\a
> > > \t) mode. That would remove the frequent conflicts on the row count and
> > > widespread changes due to changed alignment.
> > > Alternatively we could just wrap the query in \copy ... CSV.
> >
> > Hm ... yeah, it would be a good thing if changes in one view didn't so
> > frequently have ripple effects to the whole output. Not sure which
> > format is best for that though.
>
> Something like the attached maybe?

+1 (but what are those silly parens in pg_seclabels definition?),
except:

> +-- disable fancy output again
> +\a\t

Should be "enable".

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-10-25 10:50:57 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2013-10-24 09:22:52 -0400, Tom Lane wrote:
> > > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > > FWIW, I've repeatedly now thought that it'd make maintaining/updating
> > > > patches easier if we switched that query into unaligned tuple only (\a
> > > > \t) mode. That would remove the frequent conflicts on the row count and
> > > > widespread changes due to changed alignment.
> > > > Alternatively we could just wrap the query in \copy ... CSV.
> > >
> > > Hm ... yeah, it would be a good thing if changes in one view didn't so
> > > frequently have ripple effects to the whole output. Not sure which
> > > format is best for that though.
> >
> > Something like the attached maybe?
>
> +1 (but what are those silly parens in pg_seclabels definition?),

That's because it contain several UNION ALLs and ruleutils makes sure
the order is correct.

> except:
>
> > +-- disable fancy output again
> > +\a\t
>
> Should be "enable".

Hrmpf. Fixed.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> [ patch for \a\t mode in rules and sanity_check output ]

Committed with some minor adjustment of the comments.

>> +1 (but what are those silly parens in pg_seclabels definition?),

> That's because it contain several UNION ALLs and ruleutils makes sure
> the order is correct.

That looks weird to me too, but it's surely not the fault of this patch.
Maybe we should take a look at exactly what ruleutils is doing there.

regards, tom lane

On 2013-10-26 11:27:19 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > [ patch for \a\t mode in rules and sanity_check output ]
>
> Committed with some minor adjustment of the comments.

Thanks.

> >> +1 (but what are those silly parens in pg_seclabels definition?),
>
> > That's because it contain several UNION ALLs and ruleutils makes sure
> > the order is correct.
>
> That looks weird to me too, but it's surely not the fault of this patch.
> Maybe we should take a look at exactly what ruleutils is doing there.

Imo what it does looks sane - it adds parentheses whenever a child of a
set operation is a set operation again to make sure the order in which
the generated set operations are parsed/interpreted stays the same.

Now, we could probably remove that in some more cases (left is SetOp but
doesn't have an ORDER BY/LIMIT/...), but it's hard enough to figure out
when that's safe that I wouldn't bother.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-10-26 11:27:19 -0400, Tom Lane wrote:
>>> +1 (but what are those silly parens in pg_seclabels definition?),
>>
>> That looks weird to me too, but it's surely not the fault of this patch.
>> Maybe we should take a look at exactly what ruleutils is doing there.

> Imo what it does looks sane - it adds parentheses whenever a child of a
> set operation is a set operation again to make sure the order in which
> the generated set operations are parsed/interpreted stays the same.

I'm not objecting to the parens being there, but I think the layout
doesn't look nice. Not immediately sure what would look better though.
Obvious alternatives include one line per paren:

(
(
(
SELECT ...

or getting rid of the space between parens:

(((SELECT ...

but I'm not sure I'm thrilled with either of those. Thoughts?

regards, tom lane

On 2013-10-26 12:25:40 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-10-26 11:27:19 -0400, Tom Lane wrote:
> >>> +1 (but what are those silly parens in pg_seclabels definition?),
> >>
> >> That looks weird to me too, but it's surely not the fault of this patch.
> >> Maybe we should take a look at exactly what ruleutils is doing there.
>
> > Imo what it does looks sane - it adds parentheses whenever a child of a
> > set operation is a set operation again to make sure the order in which
> > the generated set operations are parsed/interpreted stays the same.
>
> I'm not objecting to the parens being there, but I think the layout
> doesn't look nice.

Ah, ok. Agreed.

> Not immediately sure what would look better though.
> Obvious alternatives include one line per paren:
>
> (
> (
> (
> SELECT ...
>
> or getting rid of the space between parens:
>
> (((SELECT ...
>
> but I'm not sure I'm thrilled with either of those. Thoughts?

ISTM indentation generally is currently so random up that all other
considerations don't play much of a role:

DROP VIEW deparse;
CREATE VIEW deparse AS SELECT 1 UNION ALL SELECT 2 UNION ALL SELECT 3;
\d+ deparse
View definition:
( SELECT 1
UNION ALL
SELECT 2)
UNION ALL
SELECT 3;

or even more extreme:

CREATE VIEW deparse AS SELECT 1 FROM pg_class JOIN pg_attribute ON (attrelid = pg_class.oid) WHERE true UNION ALL SELECT 2 FROM pg_class WHERE true UNION ALL SELECT 3 FROM pg_class WHERE true UNION (SELECT 4 FROM pg_class WHERE true ORDER BY 1 LIMIT 1) ORDER BY 1 LIMIT 10;
View definition:
( ( SELECT 1
FROM pg_class
JOIN pg_attribute ON pg_attribute.attrelid = pg_class.oid
WHERE true
UNION ALL
SELECT 2
FROM pg_class
WHERE true)
UNION ALL
SELECT 3
FROM pg_class
WHERE true)
UNION
( SELECT 4
FROM pg_class
WHERE true
ORDER BY 4::integer
LIMIT 1)
ORDER BY 1
LIMIT 10;

I don't see any consistency in that...

WRT to the parentheses around SetOps, I think they should be on their own
line. Otherwise the SELECT can wander so far right that it's hard to
correlate it to the FROM on the next line...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Oct 26, 2013 at 12:02 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Imo what it does looks sane - it adds parentheses whenever a child of a
> set operation is a set operation again to make sure the order in which
> the generated set operations are parsed/interpreted stays the same.

But UNION ALL is associative.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Oct 26, 2013 at 12:02 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Imo what it does looks sane - it adds parentheses whenever a child of a
>> set operation is a set operation again to make sure the order in which
>> the generated set operations are parsed/interpreted stays the same.

> But UNION ALL is associative.

In theory, yeah.

In practice, this could for example affect the parser's choices of
column datatypes for the UNION result. We could perhaps side-step
that by forcing datatype labeling in the UNION arms, but I'm not
prepared to bet that ruleutils' output would be right if we just
summarily removed the parentheses.

regards, tom lane

On Mon, Oct 28, 2013 at 12:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Oct 26, 2013 at 12:02 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> Imo what it does looks sane - it adds parentheses whenever a child of a
>>> set operation is a set operation again to make sure the order in which
>>> the generated set operations are parsed/interpreted stays the same.
>
>> But UNION ALL is associative.
>
> In theory, yeah.
>
> In practice, this could for example affect the parser's choices of
> column datatypes for the UNION result. We could perhaps side-step
> that by forcing datatype labeling in the UNION arms, but I'm not
> prepared to bet that ruleutils' output would be right if we just
> summarily removed the parentheses.

Well, if it were actually associative, then A UNION ALL B UNION ALL C
would be equivalent to either A UNION ALL (B UNION ALL C) or (A UNION
ALL B) UNION ALL C. But even if it's *NOT* associative, it must be
equivalent to one of those. (If not, then including the parentheses
in the output is wrong.) So we could leave the parentheses out in
whichever case it's equivalent to.

I don't feel strongly that this has to be done; it's obviously not a
project priority. But if we're uncomfortable about the way that these
constructs are being formatted during deparsing, eliminating
unnecessary nesting levels could potentially help. I fairly commonly
write queries that involve multiple UNION ALL branches and, no matter
how clever we are, having that lead to progressively deeper nesting at
each level is not going to look nice.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> ... we could leave the parentheses out in
> whichever case it's equivalent to.

Ah, I see what you're getting at now. Yeah, that might be a useful
readability improvement.

> ... I fairly commonly
> write queries that involve multiple UNION ALL branches and, no matter
> how clever we are, having that lead to progressively deeper nesting at
> each level is not going to look nice.

Agreed. I was wondering myself whether we couldn't fix things so that
all the branches are indented the same, even with parens.

regards, tom lane

On Mon, Oct 28, 2013 at 12:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> ... we could leave the parentheses out in
>> whichever case it's equivalent to.
>
> Ah, I see what you're getting at now. Yeah, that might be a useful
> readability improvement.
>
>> ... I fairly commonly
>> write queries that involve multiple UNION ALL branches and, no matter
>> how clever we are, having that lead to progressively deeper nesting at
>> each level is not going to look nice.
>
> Agreed. I was wondering myself whether we couldn't fix things so that
> all the branches are indented the same, even with parens.

Hmm, yeah, maybe. Not sure how ugly it'd be.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company