On 2020/04/03 23:55, Tom Lane wrote:
Fujii Masao <[email protected]> writes:
On 2020/04/03 22:35, Tom Lane wrote:
I think this is a bad idea. It's overcomplicated, and more to the
point: now that we've seen the problem we should realize that we're
eventually going to have failures for *any* Buffers line in text-mode
output. We're already filtering them so hard as to be nearly useless
(see a couple lines further down). I think we should just drop them
in text mode and be content with checking for them in non-text modes.
Yeah, I'm fine with the idea. The regression test for EXPLAIN has
the following four similar tests. The first one fails in regression test
because of buffers output for the planning. Your idea seems to be
equal to removal of this first test. Because there are already
the same tests with other format. Is my understanding right?
Well, there are at least three ways you could interpret my suggestion:
* don't do the test with format = text at all
* do it, but remove the buffers option
* keep test the same, make explain_filter() drop Buffers lines
But I had the last one in mind. Even if we can't *see* the output,
there is some value in making the backend run through the code ---
it'd at least catch core-dumping bugs there.
Yes, this makes sense. Thanks for clarifying that!
Or another idea is to specify "summary off" in the above first
EXPLAIN command, to suppress the summary output including
the buffers output for the planning. This seems simple and enough.
That's still assuming that only the summary Buffers line is problematic,
which I think is wrong. Keep in mind that the explain.sql test is
barely two months old; it has never seen the field, and there is no
good reason to trust that it's rock solid stable. Now that we've
seen this failure, I'm quite afraid that the existing test case
Seq Scan on int8_tbl i8 (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N
rows=N loops=N)
Buffers: shared [read]
Planning Time: N.N ms
can be made to fall over, eg by running with high enough shared_buffers.
Yeah, so I'm feeling that it's better to firstly make the explain test
more stable, separately from the patch for the planning buffers output.
Attached is the patch that changes explain.c as follows (i.e., removes
Buffers lines at all) to make it more stable.
ln := regexp_replace(ln, '\m\d+\M', 'N', 'g');
-- In sort output, the above won't match units-suffixed numbers
ln := regexp_replace(ln, '\m\d+kB', 'NkB', 'g');
- -- Text-mode buffers output varies depending on the system state
- ln := regexp_replace(ln, '^( +Buffers: shared)( hit=N)?( read=N)?',
'\1 [read]');
+ -- Ignore text-mode buffers output because it varies depending
+ -- on the system state
+ CONTINUE WHEN (ln ~ ' +Buffers: .*');
return next ln;
end loop;
end;
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/test/regress/expected/explain.out
b/src/test/regress/expected/explain.out
index 8f31c308c6..3ec66ceda3 100644
--- a/src/test/regress/expected/explain.out
+++ b/src/test/regress/expected/explain.out
@@ -20,8 +20,9 @@ begin
ln := regexp_replace(ln, '\m\d+\M', 'N', 'g');
-- In sort output, the above won't match units-suffixed numbers
ln := regexp_replace(ln, '\m\d+kB', 'NkB', 'g');
- -- Text-mode buffers output varies depending on the system state
- ln := regexp_replace(ln, '^( +Buffers: shared)( hit=N)?( read=N)?',
'\1 [read]');
+ -- Ignore text-mode buffers output because it varies depending
+ -- on the system state
+ CONTINUE WHEN (ln ~ ' +Buffers: .*');
return next ln;
end loop;
end;
@@ -71,10 +72,9 @@ select explain_filter('explain (analyze, buffers, format
text) select * from int
explain_filter
-----------------------------------------------------------------------------------------------
Seq Scan on int8_tbl i8 (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N
rows=N loops=N)
- Buffers: shared [read]
Planning Time: N.N ms
Execution Time: N.N ms
-(4 rows)
+(3 rows)
select explain_filter('explain (analyze, buffers, format json) select * from
int8_tbl i8');
explain_filter
diff --git a/src/test/regress/sql/explain.sql b/src/test/regress/sql/explain.sql
index e09371f97b..dce2a34207 100644
--- a/src/test/regress/sql/explain.sql
+++ b/src/test/regress/sql/explain.sql
@@ -22,8 +22,9 @@ begin
ln := regexp_replace(ln, '\m\d+\M', 'N', 'g');
-- In sort output, the above won't match units-suffixed numbers
ln := regexp_replace(ln, '\m\d+kB', 'NkB', 'g');
- -- Text-mode buffers output varies depending on the system state
- ln := regexp_replace(ln, '^( +Buffers: shared)( hit=N)?( read=N)?',
'\1 [read]');
+ -- Ignore text-mode buffers output because it varies depending
+ -- on the system state
+ CONTINUE WHEN (ln ~ ' +Buffers: .*');
return next ln;
end loop;
end;
