Thread: 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.Blackwell@rrd.com
http://www.rrdonnelley.com
Mike Blackwell <mike.blackwell@rrd.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.blackwell@rrd.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@2ndquadrant.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@2ndquadrant.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
Attachment
Andres Freund wrote: > On 2013-10-24 09:22:52 -0400, Tom Lane wrote: > > Andres Freund <andres@2ndquadrant.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@2ndquadrant.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
Attachment
Andres Freund <andres@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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
SELECT2 FROM pg_class WHERE true UNION ALL SELECT 3 FROM pg_class WHERE true UNION (SELECT 4 FROM pg_class WHERE true
ORDERBY 1 LIMIT 1) ORDER BY 1 LIMIT 10;
View definition: ( ( SELECT 1 FROM pg_class JOIN
pg_attributeON 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
1LIMIT10;
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@2ndquadrant.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@gmail.com> writes:
> On Sat, Oct 26, 2013 at 12:02 PM, Andres Freund <andres@2ndquadrant.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sat, Oct 26, 2013 at 12:02 PM, Andres Freund <andres@2ndquadrant.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@gmail.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.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