On Sun, Nov 14, 2021 at 8:49 PM vignesh C <vignesh21@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.