On Sun, Nov 14, 2021 at 8:49 PM vignesh C <vignes...@gmail.com> wrote: > > 7) Do we need TAP tests for this function? I think it is sufficient to > > test the function in misc_functions.sql, please remove > > 002_print_backtrace_validation.pl. Note that we don't do similar TAP > > testing for pg_log_backend_memory_contexts as well. > > I felt let's keep this test case, all the other tests just check if it > returns true or false, it does not checks for the contents in the > logfile. This is the only test which checks the logfile.
I still don't agree to have test cases within a new file 002_print_backtrace_validation.pl. I feel this test case doesn't add value because the code coverage is done by .sql test cases and .pl just ensures the backtrace appears in the server logs. I don't think we ever need a new file for this purpose. If this is the case, then there are other functions like pg_log_backend_memory_contexts or pg_log_query_plan (in progress thread) might add the same test files for the same reasons which make the TAP tests i.e. "make check-world" to take longer times. Moreover, pg_log_backend_memory_contexts has been committed without having a TAP test case. I think we can remove it. Few more comments on v11: 1) I think we can improve here by adding a link to "backend" as well, I will modify it in the other thread. + Requests to log the backtrace of the backend or the + <glossterm linkend="glossary-wal-sender">WAL sender</glossterm> or Something like: + Requests to log the backtrace of the <glossterm linkend="glossary-backend">backend</glossterm> or the + <glossterm linkend="glossary-wal-sender">WAL sender</glossterm> or 2) I think "which is enough because the target process for logging of backtrace is a backend" isn't valid anymore with 0002, righit? Please remove it. + * to call this function if we see PrintBacktracePending set. It is called from + * CHECK_FOR_INTERRUPTS() or from process specific interrupt handlers, which is + * enough because the target process for logging of backtrace is a backend. > Thanks for the comments, v11 patch attached at [1] has the changes for the > same. The v11 patches mostly look good to me except the above comments. Regards, Bharath Rupireddy.
