On 2020/04/03 23:55, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> 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
