On Mon, Nov 15, 2021 at 7:37 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> 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.
Removed
> 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
Modified
> 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.
Modified
Thanks for the comments, the attached v12 patch has the changes for the same.
Regards,
Vignesh
