Horton

Thread

Commits

  1. Improve display of query results in isolation tests.

  2. Detect unused steps in isolation specs and do some cleanup

  3. Remove dry-run mode from isolationtester

  1. Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-15T23:03:41Z

    I've been spending a lot of time looking at isolationtester results
over the past couple of days, and gotten really annoyed at how poorly
it formats query results.  In particular, any column heading or value
that is 15 characters or longer is not separated from the next column,
rendering the output quite confusing.

Attached is a little hack that tries to improve that case while making
minimal changes to the output files otherwise.

There's still a good deal to be desired here: notably, the code still
does nothing to ensure vertical alignment of successive lines when
there are wide headings or values.  But doing anything about that
would involve much-more-invasive changes of the output files.
If we wanted to buy into that, I'd think about discarding this
ad-hoc code altogether in favor of using one of libpq's fe-print.c
routines.  But I'm not really sure that the small legibility gains
that would result are worth massive changes in the output files.

Thoughts?

			regards, tom lane

  2. Re: Improving isolationtester's data output

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-15T23:20:11Z

    On 2021-Jun-15, Tom Lane wrote:

> I've been spending a lot of time looking at isolationtester results
> over the past couple of days, and gotten really annoyed at how poorly
> it formats query results.  In particular, any column heading or value
> that is 15 characters or longer is not separated from the next column,
> rendering the output quite confusing.

Yeah, I noticed this too.

> Attached is a little hack that tries to improve that case while making
> minimal changes to the output files otherwise.

Seems pretty reasonable.

> There's still a good deal to be desired here: notably, the code still
> does nothing to ensure vertical alignment of successive lines when
> there are wide headings or values.  But doing anything about that
> would involve much-more-invasive changes of the output files.
> If we wanted to buy into that, I'd think about discarding this
> ad-hoc code altogether in favor of using one of libpq's fe-print.c
> routines.  But I'm not really sure that the small legibility gains
> that would result are worth massive changes in the output files.

Shrug -- it's a one time change.  It wouldn't bother me, for one.

-- 
Álvaro Herrera       Valdivia, Chile
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)

  3. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-15T23:26:25Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jun-15, Tom Lane wrote:
>> If we wanted to buy into that, I'd think about discarding this
>> ad-hoc code altogether in favor of using one of libpq's fe-print.c
>> routines.  But I'm not really sure that the small legibility gains
>> that would result are worth massive changes in the output files.

> Shrug -- it's a one time change.  It wouldn't bother me, for one.

Going forward it wouldn't be a problem, but back-patching isolation
test cases might find it annoying.  On the other hand, my nearby
patch to improve isolation test stability is already going to create
issues of that sort.  (Unless, dare I say it, we back-patch that.)

I do find it a bit attractive to create some regression-testing
coverage of fe-print.c.  We are never going to remove that code,
AFAICS, so getting some benefit from it would be nice.

			regards, tom lane

  4. Re: Improving isolationtester's data output

    Andres Freund <andres@anarazel.de> — 2021-06-16T01:31:15Z

    Hi,

On 2021-06-15 19:26:25 -0400, Tom Lane wrote:
> Going forward it wouldn't be a problem, but back-patching isolation
> test cases might find it annoying.  On the other hand, my nearby
> patch to improve isolation test stability is already going to create
> issues of that sort.  (Unless, dare I say it, we back-patch that.)

It might be worth to back-patch - aren't there some back branch cases of
test instability? And perhaps more importantly, I'm sure we'll encounter
cases of writing new isolation tests in the course of fixing bugs that
we'd want to backpatch that are hard to make reliable without the new
features?

Greetings,

Andres Freund

  5. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-16T01:43:31Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-06-15 19:26:25 -0400, Tom Lane wrote:
>> Going forward it wouldn't be a problem, but back-patching isolation
>> test cases might find it annoying.  On the other hand, my nearby
>> patch to improve isolation test stability is already going to create
>> issues of that sort.  (Unless, dare I say it, we back-patch that.)

> It might be worth to back-patch - aren't there some back branch cases of
> test instability? And perhaps more importantly, I'm sure we'll encounter
> cases of writing new isolation tests in the course of fixing bugs that
> we'd want to backpatch that are hard to make reliable without the new
> features?

Yeah, there absolutely is a case to back-patch things like this.  Whether
it's a strong enough case, I dunno.  I'm probably too close to the patch
to have an unbiased opinion about that.

However, a quick look through the commit history finds several places
where we complained about not being able to back-patch isolation tests to
before 9.6 because we hadn't back-patched that version's isolationtester
improvements.  I found 6b802cfc7, 790026972, c88411995, 8b21b416e without
looking too hard.  So that history certainly suggests that not
back-patching such test infrastructure is the Wrong Thing.

(And yeah, the failures we complained of in the other thread are
certainly there in the back branches.  I think the only reason there
seem to be fewer is that the back branches see fewer test runs.)

			regards, tom lane

  6. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-16T02:44:29Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jun-15, Tom Lane wrote:
>> If we wanted to buy into that, I'd think about discarding this
>> ad-hoc code altogether in favor of using one of libpq's fe-print.c
>> routines.  But I'm not really sure that the small legibility gains
>> that would result are worth massive changes in the output files.

> Shrug -- it's a one time change.  It wouldn't bother me, for one.

Here's a really quick-and-dirty patch to see what that would look
like.  I haven't bothered here to update the expected-files outside
the main src/test/isolation directory, nor to fix the variant files.

			regards, tom lane

  7. Re: Improving isolationtester's data output

    Noah Misch <noah@leadboat.com> — 2021-06-16T03:43:23Z

    On Tue, Jun 15, 2021 at 09:43:31PM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2021-06-15 19:26:25 -0400, Tom Lane wrote:
> >> Going forward it wouldn't be a problem, but back-patching isolation
> >> test cases might find it annoying.  On the other hand, my nearby
> >> patch to improve isolation test stability is already going to create
> >> issues of that sort.  (Unless, dare I say it, we back-patch that.)
> 
> > It might be worth to back-patch - aren't there some back branch cases of
> > test instability? And perhaps more importantly, I'm sure we'll encounter
> > cases of writing new isolation tests in the course of fixing bugs that
> > we'd want to backpatch that are hard to make reliable without the new
> > features?
> 
> Yeah, there absolutely is a case to back-patch things like this.  Whether
> it's a strong enough case, I dunno.  I'm probably too close to the patch
> to have an unbiased opinion about that.
> 
> However, a quick look through the commit history finds several places
> where we complained about not being able to back-patch isolation tests to
> before 9.6 because we hadn't back-patched that version's isolationtester
> improvements.  I found 6b802cfc7, 790026972, c88411995, 8b21b416e without
> looking too hard.  So that history certainly suggests that not
> back-patching such test infrastructure is the Wrong Thing.

I'm +1 for back-patching this class of change.  I've wasted time adapting a
back-patch's test case to account for non-back-patched test infrastructure
changes.  Every back-patch of test infrastructure has been a strict win from
my perspective.

  8. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-16T14:03:52Z

    Noah Misch <noah@leadboat.com> writes:
> I'm +1 for back-patching this class of change.  I've wasted time adapting a
> back-patch's test case to account for non-back-patched test infrastructure
> changes.  Every back-patch of test infrastructure has been a strict win from
> my perspective.

Hearing few objections, I'll plan on back-patching.  I'm thinking that the
best thing to do is apply these changes after beta2 wraps, but before we
branch v14.  Waiting till after the branch would just create duplicate
work.

BTW, as long as we're thinking of back-patching nontrivial specfile
changes, I have another modest proposal.  What do people think of
removing the requirement for step/session names to be double-quoted,
and instead letting them work like SQL identifiers?  A quick grep
shows that practically all the existing names are plain identifiers,
so we could just drop their quotes for a useful notational savings.
While I haven't actually tried yet, I doubt it'd be hard to adopt
scan.l's identifier rules into specscanner.l.  (Probably wouldn't
bother with auto case-folding, though.)

			regards, tom lane

  9. Re: Improving isolationtester's data output

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-16T15:14:55Z

    On 2021-Jun-16, Tom Lane wrote:

> Noah Misch <noah@leadboat.com> writes:
> > I'm +1 for back-patching this class of change.  I've wasted time adapting a
> > back-patch's test case to account for non-back-patched test infrastructure
> > changes.  Every back-patch of test infrastructure has been a strict win from
> > my perspective.
> 
> Hearing few objections, I'll plan on back-patching.  I'm thinking that the
> best thing to do is apply these changes after beta2 wraps, but before we
> branch v14.

Great.

> BTW, as long as we're thinking of back-patching nontrivial specfile
> changes, I have another modest proposal.  What do people think of
> removing the requirement for step/session names to be double-quoted,
> and instead letting them work like SQL identifiers?

Yes *please*.

-- 
Álvaro Herrera       Valdivia, Chile
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere."                (Lamar Owen)

  10. Re: Improving isolationtester's data output

    Andres Freund <andres@anarazel.de> — 2021-06-16T19:30:23Z

    Hi,

On 2021-06-15 22:44:29 -0400, Tom Lane wrote:
> Here's a really quick-and-dirty patch to see what that would look
> like.  I haven't bothered here to update the expected-files outside
> the main src/test/isolation directory, nor to fix the variant files.

Neat.


> +	memset(&popt, 0, sizeof(popt));
> +	popt.header = true;
> +	popt.align = true;
> +	popt.fieldSep = "|";
> +	PQprint(stdout, res, &popt);
>  }

Is there an argument for not aligning because that can make diffs larger
than the actual data changes? E.g. one row being longer will cause all
rows in the result set to be shown as differing because of the added
padding?  This has been a problem in the normal regression tests, where
we solved it by locally disabling alignment. It might be unproblematic
for isolationtester, because we don't often have large result sets...

Greetings,

Andres Freund

  11. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-16T19:33:29Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jun-16, Tom Lane wrote:
>> Hearing few objections, I'll plan on back-patching.  I'm thinking that the
>> best thing to do is apply these changes after beta2 wraps, but before we
>> branch v14.

> Great.

After checking cross-version diffs to see how painful that is likely
to be, I'm inclined to also back-patch Michael's v13 commits

989d23b04beac0c26f44c379b04ac781eaa4265e
    Detect unused steps in isolation specs and do some cleanup

9903338b5ea59093d77cfe50ec0b1c22d4a7d843
    Remove dry-run mode from isolationtester

as those touched some of the same code areas, and it doesn't seem like
there'd be any harm in making these aspects uniform across all the
branches.  If Michael wants to do that back-patching himself, that's
fine with me, otherwise I'll do it.

Also, having slept on it, I'm leaning towards to the approach of
using PQprint() instead of just tweaking the existing code.  At first
I thought that was too much churn in the output files, but it really
does seem to make them significantly more readable.

			regards, tom lane

  12. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-16T19:37:16Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-06-15 22:44:29 -0400, Tom Lane wrote:
>> +	memset(&popt, 0, sizeof(popt));
>> +	popt.header = true;
>> +	popt.align = true;
>> +	popt.fieldSep = "|";
>> +	PQprint(stdout, res, &popt);

> Is there an argument for not aligning because that can make diffs larger
> than the actual data changes? E.g. one row being longer will cause all
> rows in the result set to be shown as differing because of the added
> padding?  This has been a problem in the normal regression tests, where
> we solved it by locally disabling alignment. It might be unproblematic
> for isolationtester, because we don't often have large result sets...

I tried it that way first, and didn't much like the look of it.

I think the result sets in the isolation tests don't have a big
problem here: as you say, they aren't very large, and in most of them
the column widths are fairly uniform anyway.

			regards, tom lane

  13. Re: Improving isolationtester's data output

    Andres Freund <andres@anarazel.de> — 2021-06-16T19:42:57Z

    Hi,

On Wed, Jun 16, 2021, at 12:37, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2021-06-15 22:44:29 -0400, Tom Lane wrote:
> >> +	memset(&popt, 0, sizeof(popt));
> >> +	popt.header = true;
> >> +	popt.align = true;
> >> +	popt.fieldSep = "|";
> >> +	PQprint(stdout, res, &popt);
> 
> > Is there an argument for not aligning because that can make diffs larger
> > than the actual data changes? E.g. one row being longer will cause all
> > rows in the result set to be shown as differing because of the added
> > padding?  This has been a problem in the normal regression tests, where
> > we solved it by locally disabling alignment. It might be unproblematic
> > for isolationtester, because we don't often have large result sets...
> 
> I tried it that way first, and didn't much like the look of it.
> 
> I think the result sets in the isolation tests don't have a big
> problem here: as you say, they aren't very large, and in most of them
> the column widths are fairly uniform anyway.

Cool. Just wanted to be sure we considered it.

Andres

  14. Re: Improving isolationtester's data output

    Michael Paquier <michael@paquier.xyz> — 2021-06-17T00:16:01Z

    On Wed, Jun 16, 2021 at 03:33:29PM -0400, Tom Lane wrote:
> After checking cross-version diffs to see how painful that is likely
> to be, I'm inclined to also back-patch Michael's v13 commits
> 
> 989d23b04beac0c26f44c379b04ac781eaa4265e
>     Detect unused steps in isolation specs and do some cleanup
> 
> 9903338b5ea59093d77cfe50ec0b1c22d4a7d843
>     Remove dry-run mode from isolationtester
> 
> as those touched some of the same code areas, and it doesn't seem like
> there'd be any harm in making these aspects uniform across all the
> branches.  If Michael wants to do that back-patching himself, that's
> fine with me, otherwise I'll do it.

There may be tests in stable branches that define steps remaining
unused, but that's a minimal risk.  Down to which version do you need
these?  All the way down to 9.6?
--
Michael

  15. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-17T01:10:25Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jun 16, 2021 at 03:33:29PM -0400, Tom Lane wrote:
>> After checking cross-version diffs to see how painful that is likely
>> to be, I'm inclined to also back-patch Michael's v13 commits
>> 989d23b04beac0c26f44c379b04ac781eaa4265e
>> Detect unused steps in isolation specs and do some cleanup
>> 9903338b5ea59093d77cfe50ec0b1c22d4a7d843
>> Remove dry-run mode from isolationtester

> There may be tests in stable branches that define steps remaining
> unused, but that's a minimal risk.

Yeah, it only results in a message in the output file anyway.

> Down to which version do you need
> these?  All the way down to 9.6?

Yes please.

			regards, tom lane

  16. Re: Improving isolationtester's data output

    Michael Paquier <michael@paquier.xyz> — 2021-06-17T03:01:31Z

    On Wed, Jun 16, 2021 at 09:10:25PM -0400, Tom Lane wrote:
> Yeah, it only results in a message in the output file anyway.

That itself would blow up the buildfarm, as 06fdc4e has proved.

> Yes please.

Nobody has complained about the removal of --dry-run with 13~.  The
second one would cause tests to fail after a minor upgrade for
extensions using isolationtester, but it seems like a good thing to
inform people about anyway.  So, okay, both parts are done.
--
Michael

  17. Re: Improving isolationtester's data output

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-17T03:34:57Z

    Michael Paquier <michael@paquier.xyz> writes:
> Nobody has complained about the removal of --dry-run with 13~.  The
> second one would cause tests to fail after a minor upgrade for
> extensions using isolationtester, but it seems like a good thing to
> inform people about anyway.  So, okay, both parts are done.

Thanks!

			regards, tom lane