Improving isolationtester's data output

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)

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

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

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

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

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.

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

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)

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

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

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

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

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

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

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

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