From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@postgresql.org>
Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Subject: Re: pg_partition_tree crashes for a non-defined relation
Date: Sun, 9 Dec 2018 13:54:52 -0500
Message-ID: <20181209185452.GF3415@tamriel.snowman.net> (raw)
In-Reply-To: <16590.1544378743@sss.pgh.pa.us>
References: <20181207010406.GO2407@paquier.xyz>
<20181207230540.GA12740@paquier.xyz>
<6105.1544243612@sss.pgh.pa.us>
<20181208065436.GC1989@paquier.xyz>
<20181208134608.GY3415@tamriel.snowman.net>
<20181208231507.GA1833@paquier.xyz>
<20181209174815.GD3415@tamriel.snowman.net>
<16590.1544378743@sss.pgh.pa.us>
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Michael Paquier (michael@paquier.xyz) wrote:
> >> On Sat, Dec 08, 2018 at 08:46:08AM -0500, Stephen Frost wrote:
> >>> I wonder if we maybe should have a regression test for every such
> >>> function which just queries the catalog in a way to force the function
> >>> to be called for every relation defined in the regression tests, to
> >>> ensure that it doesn't segfault or throw an error..
>
> > No, I mean something like:
> > with x as (select pg_partition_tree(relname) from pg_class)
> > select 1 from x limit 1;
> > or whatever it takes to make sure that the function is run against every
> > entry in pg_class (or whatever is appropriate) while not returning the
> > results (since we don't actually care about the output, we just want to
> > make sure it doesn't ERROR or crash).
>
> I'm going to object to that on cost grounds. It is not reasonable to
> run moderately-expensive functions like this on more than a thousand
> pg_class entries in order to test what are just a few distinct cases,
> especially in code that's highly unlikely to break once written.
Yeah, I had been wondering if it would be too expensive also.
I don't entirely buy off on the argument that it's code that's 'highly
unlikely to break once written' though- we do add new relkinds from time
to time, for example. Perhaps we could have these functions run just
once per relkind.
> Moreover, this would only help check a specific new function if the
> author or reviewer remembered to add a test case that does it.
We could possibly write a test which would run every function that
accepts the typical data types (oid/regclass/name/etc) instead of
depending on the author to remember to add it.
> Since the whole problem here is "I forgot to consider this", I don't
> have much faith in that happening.
Yeah, I agree that we'd want something more automated as, otherwise, it
would definitely be likely to be forgotten.
> Maybe we should have some sort of checklist of things to think about
> when adding new SQL-visible functions, rather than trying to
> memorialize every such consideration as a regression test no matter
> how expensive. Admittedly, "I forgot to go over the checklist" is
> still a problem; but at least it's not penalizing every developer and
> every buildfarm run till kingdom come.
This seems like something we should do regardless. Moreover, I'd
suggest that we start documenting some of these policies in the way that
we have a style guide for errors and such- we need a "system function
style guide" that could start with something like "prefer to return NULL
instead of ERROR on unexpected but otherwise valid inputs, and test that
the code doesn't segfault when given a variety of inputs".
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../20181209185452.GF3415@tamriel.snowman.net/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, tgl@sss.pgh.pa.us, michael@paquier.xyz, Langote_Amit_f8@lab.ntt.co.jp
Subject: Re: pg_partition_tree crashes for a non-defined relation
In-Reply-To: <20181209185452.GF3415@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox