Add test module for verifying backtrace functionality
Hi,
Postgres has a good amount of code for dealing with backtraces - two
GUCs backtrace_functions and backtrace_on_internal_error,
errbacktrace; all of which use core function set_backtrace from
elog.c. I've not seen this code being tested at all, see code coverage
report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.
I think adding a simple test module (containing no .c files) with only
TAP tests will help cover this code. I ended up having it as a
separate module under src/test/modules/test_backtrace as I was not
able to find an existing TAP file in src/test to add these tests. I'm
able to verify the backtrace related code with the attached patch
consistently. The TAP tests rely on the fact that the server emits
text "BACKTRACE: " to server logs before logging the backtrace, and
the backtrace contains the function name in which the error occurs.
I've turned off query statement logging (set log_statement = none,
log_min_error_statement = fatal) so that the tests get to see the
functions only in the backtrace. Although the CF bot is happy with the
attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
there might be some more flakiness to it.
Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Add-test-module-for-backtrace-functionality.patchapplication/x-patch; name=v1-0001-Add-test-module-for-backtrace-functionality.patchDownload
From 35d716f87611439d56f233a26f29a3b582ed3fc0 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 12 Feb 2024 15:22:05 +0000
Subject: [PATCH v1] Add test module for backtrace functionality
---
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
src/test/modules/test_backtrace/.gitignore | 4 +
src/test/modules/test_backtrace/Makefile | 14 +++
src/test/modules/test_backtrace/README | 4 +
src/test/modules/test_backtrace/meson.build | 12 ++
.../modules/test_backtrace/t/001_backtrace.pl | 103 ++++++++++++++++++
7 files changed, 139 insertions(+)
create mode 100644 src/test/modules/test_backtrace/.gitignore
create mode 100644 src/test/modules/test_backtrace/Makefile
create mode 100644 src/test/modules/test_backtrace/README
create mode 100644 src/test/modules/test_backtrace/meson.build
create mode 100644 src/test/modules/test_backtrace/t/001_backtrace.pl
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 89aa41b5e3..3967311048 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -13,6 +13,7 @@ SUBDIRS = \
libpq_pipeline \
plsample \
spgist_name_ops \
+ test_backtrace \
test_bloomfilter \
test_copy_callbacks \
test_custom_rmgrs \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 8fbe742d38..3b21b389af 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -12,6 +12,7 @@ subdir('libpq_pipeline')
subdir('plsample')
subdir('spgist_name_ops')
subdir('ssl_passphrase_callback')
+subdir('test_backtrace')
subdir('test_bloomfilter')
subdir('test_copy_callbacks')
subdir('test_custom_rmgrs')
diff --git a/src/test/modules/test_backtrace/.gitignore b/src/test/modules/test_backtrace/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_backtrace/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_backtrace/Makefile b/src/test/modules/test_backtrace/Makefile
new file mode 100644
index 0000000000..b908864e89
--- /dev/null
+++ b/src/test/modules/test_backtrace/Makefile
@@ -0,0 +1,14 @@
+# src/test/modules/test_backtrace/Makefile
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_backtrace
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_backtrace/README b/src/test/modules/test_backtrace/README
new file mode 100644
index 0000000000..ae1366bd58
--- /dev/null
+++ b/src/test/modules/test_backtrace/README
@@ -0,0 +1,4 @@
+This directory doesn't actually contain any extension module.
+
+Instead it is a home for backtrace tests that we want to run using TAP
+infrastructure.
diff --git a/src/test/modules/test_backtrace/meson.build b/src/test/modules/test_backtrace/meson.build
new file mode 100644
index 0000000000..4adffcc914
--- /dev/null
+++ b/src/test/modules/test_backtrace/meson.build
@@ -0,0 +1,12 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+tests += {
+ 'name': 'test_backtrace',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+ 'tests': [
+ 't/001_backtrace.pl',
+ ],
+ },
+}
diff --git a/src/test/modules/test_backtrace/t/001_backtrace.pl b/src/test/modules/test_backtrace/t/001_backtrace.pl
new file mode 100644
index 0000000000..ecd53dbe7b
--- /dev/null
+++ b/src/test/modules/test_backtrace/t/001_backtrace.pl
@@ -0,0 +1,103 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test PostgreSQL backtrace related code.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+
+# Turn off query statement logging so that we get to see the functions in only
+# backtraces.
+$node->append_conf(
+ 'postgresql.conf', qq{
+backtrace_functions = 'pg_terminate_backend, pg_create_restore_point'
+wal_level = replica
+log_statement = none
+log_min_error_statement = fatal
+});
+$node->start;
+
+# Check if backtrace generation is supported on this installation.
+my ($result, $stdout, $stderr);
+my $log_offset = -s $node->logfile;
+
+# Generate an error with negative timeout.
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_terminate_backend(123, -1);
+));
+
+if ($stderr =~ m/"timeout" must not be negative/ &&
+ $node->log_contains(
+ qr/backtrace generation is not supported by this installation/,
+ $log_offset))
+{
+ plan skip_all => 'backtrace generation is not supported by this installation';
+}
+
+# Start verifying for the results only after skip_all check.
+ok( $stderr =~ m/"timeout" must not be negative/,
+ 'error from terminating backend is logged');
+
+my $backtrace_text = qr/BACKTRACE: /;
+
+ok( $node->log_contains(
+ $backtrace_text, $log_offset),
+ "backtrace pg_terminate_backend start is found");
+
+ok( $node->log_contains(
+ "pg_terminate_backend", $log_offset),
+ "backtrace pg_terminate_backend is found");
+
+$log_offset = -s $node->logfile;
+
+# Generate an error with a long restore point name.
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_create_restore_point(repeat('A', 1024));
+));
+ok( $stderr =~ m/value too long for restore point \(maximum .* characters\)/,
+ 'error from restore point function is logged');
+
+ok( $node->log_contains(
+ $backtrace_text, $log_offset),
+ "backtrace pg_create_restore_point start is found");
+
+ok( $node->log_contains(
+ "pg_create_restore_point", $log_offset),
+ "backtrace pg_create_restore_point is found");
+
+# Test if backtrace gets generated on internal errors when
+# backtrace_on_internal_error is set. Note that we use a function (i.e.
+# pg_replication_slot_advance) that generates an error with ereport without
+# setting errcode explicitly, in which case elog.c assumes it as an internal
+# error (see edata->sqlerrcode = ERRCODE_INTERNAL_ERROR; in errstart() in
+# elog.c).
+$node->append_conf('postgresql.conf', "backtrace_on_internal_error = on");
+$node->reload;
+
+$node->safe_psql('postgres',
+ "SELECT pg_create_physical_replication_slot('myslot', true);");
+
+$log_offset = -s $node->logfile;
+
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_replication_slot_advance('myslot', '0/0'::pg_lsn);
+));
+ok( $stderr =~ m/invalid target WAL LSN/,
+ 'error from replication slot advance is logged');
+
+ok( $node->log_contains(
+ $backtrace_text, $log_offset),
+ "backtrace pg_replication_slot_advance start is found");
+
+ok( $node->log_contains(
+ "pg_replication_slot_advance", $log_offset),
+ "backtrace pg_replication_slot_advance is found");
+
+done_testing();
--
2.34.1
On Tue, Feb 13, 2024 at 2:11 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Hi,
Postgres has a good amount of code for dealing with backtraces - two
GUCs backtrace_functions and backtrace_on_internal_error,
errbacktrace; all of which use core function set_backtrace from
elog.c. I've not seen this code being tested at all, see code coverage
report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.I think adding a simple test module (containing no .c files) with only
TAP tests will help cover this code. I ended up having it as a
separate module under src/test/modules/test_backtrace as I was not
able to find an existing TAP file in src/test to add these tests. I'm
able to verify the backtrace related code with the attached patch
consistently. The TAP tests rely on the fact that the server emits
text "BACKTRACE: " to server logs before logging the backtrace, and
the backtrace contains the function name in which the error occurs.
I've turned off query statement logging (set log_statement = none,
log_min_error_statement = fatal) so that the tests get to see the
functions only in the backtrace. Although the CF bot is happy with the
attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
there might be some more flakiness to it.Thoughts?
Ran pgperltidy on the new TAP test file added. Please see the attached v2 patch.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Add-test-module-for-backtrace-functionality.patchapplication/x-patch; name=v2-0001-Add-test-module-for-backtrace-functionality.patchDownload
From 2cfe9c87dea980efa2f319e9c2e873e83e561b9e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 20 Feb 2024 05:48:41 +0000
Subject: [PATCH v2] Add test module for backtrace functionality
---
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
src/test/modules/test_backtrace/.gitignore | 4 +
src/test/modules/test_backtrace/Makefile | 14 +++
src/test/modules/test_backtrace/README | 4 +
src/test/modules/test_backtrace/meson.build | 12 +++
.../modules/test_backtrace/t/001_backtrace.pl | 98 +++++++++++++++++++
7 files changed, 134 insertions(+)
create mode 100644 src/test/modules/test_backtrace/.gitignore
create mode 100644 src/test/modules/test_backtrace/Makefile
create mode 100644 src/test/modules/test_backtrace/README
create mode 100644 src/test/modules/test_backtrace/meson.build
create mode 100644 src/test/modules/test_backtrace/t/001_backtrace.pl
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 89aa41b5e3..3967311048 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -13,6 +13,7 @@ SUBDIRS = \
libpq_pipeline \
plsample \
spgist_name_ops \
+ test_backtrace \
test_bloomfilter \
test_copy_callbacks \
test_custom_rmgrs \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 8fbe742d38..3b21b389af 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -12,6 +12,7 @@ subdir('libpq_pipeline')
subdir('plsample')
subdir('spgist_name_ops')
subdir('ssl_passphrase_callback')
+subdir('test_backtrace')
subdir('test_bloomfilter')
subdir('test_copy_callbacks')
subdir('test_custom_rmgrs')
diff --git a/src/test/modules/test_backtrace/.gitignore b/src/test/modules/test_backtrace/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_backtrace/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_backtrace/Makefile b/src/test/modules/test_backtrace/Makefile
new file mode 100644
index 0000000000..b908864e89
--- /dev/null
+++ b/src/test/modules/test_backtrace/Makefile
@@ -0,0 +1,14 @@
+# src/test/modules/test_backtrace/Makefile
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_backtrace
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_backtrace/README b/src/test/modules/test_backtrace/README
new file mode 100644
index 0000000000..ae1366bd58
--- /dev/null
+++ b/src/test/modules/test_backtrace/README
@@ -0,0 +1,4 @@
+This directory doesn't actually contain any extension module.
+
+Instead it is a home for backtrace tests that we want to run using TAP
+infrastructure.
diff --git a/src/test/modules/test_backtrace/meson.build b/src/test/modules/test_backtrace/meson.build
new file mode 100644
index 0000000000..4adffcc914
--- /dev/null
+++ b/src/test/modules/test_backtrace/meson.build
@@ -0,0 +1,12 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+tests += {
+ 'name': 'test_backtrace',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+ 'tests': [
+ 't/001_backtrace.pl',
+ ],
+ },
+}
diff --git a/src/test/modules/test_backtrace/t/001_backtrace.pl b/src/test/modules/test_backtrace/t/001_backtrace.pl
new file mode 100644
index 0000000000..414af436ae
--- /dev/null
+++ b/src/test/modules/test_backtrace/t/001_backtrace.pl
@@ -0,0 +1,98 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test PostgreSQL backtrace related code.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+
+# Turn off query statement logging so that we get to see the functions in only
+# backtraces.
+$node->append_conf(
+ 'postgresql.conf', qq{
+backtrace_functions = 'pg_terminate_backend, pg_create_restore_point'
+wal_level = replica
+log_statement = none
+log_min_error_statement = fatal
+});
+$node->start;
+
+# Check if backtrace generation is supported on this installation.
+my ($result, $stdout, $stderr);
+my $log_offset = -s $node->logfile;
+
+# Generate an error with negative timeout.
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_terminate_backend(123, -1);
+));
+
+if ($stderr =~ m/"timeout" must not be negative/
+ && $node->log_contains(
+ qr/backtrace generation is not supported by this installation/,
+ $log_offset))
+{
+ plan skip_all =>
+ 'backtrace generation is not supported by this installation';
+}
+
+# Start verifying for the results only after skip_all check.
+ok( $stderr =~ m/"timeout" must not be negative/,
+ 'error from terminating backend is logged');
+
+my $backtrace_text = qr/BACKTRACE: /;
+
+ok( $node->log_contains($backtrace_text, $log_offset),
+ "backtrace pg_terminate_backend start is found");
+
+ok($node->log_contains("pg_terminate_backend", $log_offset),
+ "backtrace pg_terminate_backend is found");
+
+$log_offset = -s $node->logfile;
+
+# Generate an error with a long restore point name.
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_create_restore_point(repeat('A', 1024));
+));
+ok( $stderr =~ m/value too long for restore point \(maximum .* characters\)/,
+ 'error from restore point function is logged');
+
+ok( $node->log_contains($backtrace_text, $log_offset),
+ "backtrace pg_create_restore_point start is found");
+
+ok($node->log_contains("pg_create_restore_point", $log_offset),
+ "backtrace pg_create_restore_point is found");
+
+# Test if backtrace gets generated on internal errors when
+# backtrace_on_internal_error is set. Note that we use a function (i.e.
+# pg_replication_slot_advance) that generates an error with ereport without
+# setting errcode explicitly, in which case elog.c assumes it as an internal
+# error (see edata->sqlerrcode = ERRCODE_INTERNAL_ERROR; in errstart() in
+# elog.c).
+$node->append_conf('postgresql.conf', "backtrace_on_internal_error = on");
+$node->reload;
+
+$node->safe_psql('postgres',
+ "SELECT pg_create_physical_replication_slot('myslot', true);");
+
+$log_offset = -s $node->logfile;
+
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_replication_slot_advance('myslot', '0/0'::pg_lsn);
+));
+ok($stderr =~ m/invalid target WAL LSN/,
+ 'error from replication slot advance is logged');
+
+ok( $node->log_contains($backtrace_text, $log_offset),
+ "backtrace pg_replication_slot_advance start is found");
+
+ok($node->log_contains("pg_replication_slot_advance", $log_offset),
+ "backtrace pg_replication_slot_advance is found");
+
+done_testing();
--
2.34.1
On Tue, Feb 20, 2024 at 11:30 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Tue, Feb 13, 2024 at 2:11 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
Postgres has a good amount of code for dealing with backtraces - two
GUCs backtrace_functions and backtrace_on_internal_error,
errbacktrace; all of which use core function set_backtrace from
elog.c. I've not seen this code being tested at all, see code coverage
report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.I think adding a simple test module (containing no .c files) with only
TAP tests will help cover this code. I ended up having it as a
separate module under src/test/modules/test_backtrace as I was not
able to find an existing TAP file in src/test to add these tests. I'm
able to verify the backtrace related code with the attached patch
consistently. The TAP tests rely on the fact that the server emits
text "BACKTRACE: " to server logs before logging the backtrace, and
the backtrace contains the function name in which the error occurs.
I've turned off query statement logging (set log_statement = none,
log_min_error_statement = fatal) so that the tests get to see the
functions only in the backtrace. Although the CF bot is happy with the
attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
there might be some more flakiness to it.Thoughts?
Ran pgperltidy on the new TAP test file added. Please see the attached v2 patch.
I've now moved the new TAP test file to src/test/modules/test_misc/t
as opposed to a new test module to keep it simple. I was not sure why
I hadn't done that in the first place.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v3-0001-Add-TAP-tests-for-backtrace-functionality.patchapplication/octet-stream; name=v3-0001-Add-TAP-tests-for-backtrace-functionality.patchDownload
From bdf22ec48f7edb32fb5b24addc32ab433fd1f35e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 16 Mar 2024 04:10:32 +0000
Subject: [PATCH v3] Add TAP tests for backtrace functionality
---
src/test/modules/test_misc/meson.build | 3 +-
src/test/modules/test_misc/t/006_backtrace.pl | 98 +++++++++++++++++++
2 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/test_misc/t/006_backtrace.pl
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index df2913e893..0e0406de95 100644
--- a/src/test/modules/test_misc/meson.build
+++ b/src/test/modules/test_misc/meson.build
@@ -13,7 +13,8 @@ tests += {
't/002_tablespace.pl',
't/003_check_guc.pl',
't/004_io_direct.pl',
- 't/005_timeouts.pl'
+ 't/005_timeouts.pl',
+ 't/006_backtrace.pl'
],
},
}
diff --git a/src/test/modules/test_misc/t/006_backtrace.pl b/src/test/modules/test_misc/t/006_backtrace.pl
new file mode 100644
index 0000000000..414af436ae
--- /dev/null
+++ b/src/test/modules/test_misc/t/006_backtrace.pl
@@ -0,0 +1,98 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test PostgreSQL backtrace related code.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+
+# Turn off query statement logging so that we get to see the functions in only
+# backtraces.
+$node->append_conf(
+ 'postgresql.conf', qq{
+backtrace_functions = 'pg_terminate_backend, pg_create_restore_point'
+wal_level = replica
+log_statement = none
+log_min_error_statement = fatal
+});
+$node->start;
+
+# Check if backtrace generation is supported on this installation.
+my ($result, $stdout, $stderr);
+my $log_offset = -s $node->logfile;
+
+# Generate an error with negative timeout.
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_terminate_backend(123, -1);
+));
+
+if ($stderr =~ m/"timeout" must not be negative/
+ && $node->log_contains(
+ qr/backtrace generation is not supported by this installation/,
+ $log_offset))
+{
+ plan skip_all =>
+ 'backtrace generation is not supported by this installation';
+}
+
+# Start verifying for the results only after skip_all check.
+ok( $stderr =~ m/"timeout" must not be negative/,
+ 'error from terminating backend is logged');
+
+my $backtrace_text = qr/BACKTRACE: /;
+
+ok( $node->log_contains($backtrace_text, $log_offset),
+ "backtrace pg_terminate_backend start is found");
+
+ok($node->log_contains("pg_terminate_backend", $log_offset),
+ "backtrace pg_terminate_backend is found");
+
+$log_offset = -s $node->logfile;
+
+# Generate an error with a long restore point name.
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_create_restore_point(repeat('A', 1024));
+));
+ok( $stderr =~ m/value too long for restore point \(maximum .* characters\)/,
+ 'error from restore point function is logged');
+
+ok( $node->log_contains($backtrace_text, $log_offset),
+ "backtrace pg_create_restore_point start is found");
+
+ok($node->log_contains("pg_create_restore_point", $log_offset),
+ "backtrace pg_create_restore_point is found");
+
+# Test if backtrace gets generated on internal errors when
+# backtrace_on_internal_error is set. Note that we use a function (i.e.
+# pg_replication_slot_advance) that generates an error with ereport without
+# setting errcode explicitly, in which case elog.c assumes it as an internal
+# error (see edata->sqlerrcode = ERRCODE_INTERNAL_ERROR; in errstart() in
+# elog.c).
+$node->append_conf('postgresql.conf', "backtrace_on_internal_error = on");
+$node->reload;
+
+$node->safe_psql('postgres',
+ "SELECT pg_create_physical_replication_slot('myslot', true);");
+
+$log_offset = -s $node->logfile;
+
+($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT pg_replication_slot_advance('myslot', '0/0'::pg_lsn);
+));
+ok($stderr =~ m/invalid target WAL LSN/,
+ 'error from replication slot advance is logged');
+
+ok( $node->log_contains($backtrace_text, $log_offset),
+ "backtrace pg_replication_slot_advance start is found");
+
+ok($node->log_contains("pg_replication_slot_advance", $log_offset),
+ "backtrace pg_replication_slot_advance is found");
+
+done_testing();
--
2.34.1
On 16.03.24 05:25, Bharath Rupireddy wrote:
Postgres has a good amount of code for dealing with backtraces - two
GUCs backtrace_functions and backtrace_on_internal_error,
errbacktrace; all of which use core function set_backtrace from
elog.c. I've not seen this code being tested at all, see code coverage
report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.I think adding a simple test module (containing no .c files) with only
TAP tests will help cover this code. I ended up having it as a
separate module under src/test/modules/test_backtrace as I was not
able to find an existing TAP file in src/test to add these tests. I'm
able to verify the backtrace related code with the attached patch
consistently. The TAP tests rely on the fact that the server emits
text "BACKTRACE: " to server logs before logging the backtrace, and
the backtrace contains the function name in which the error occurs.
I've turned off query statement logging (set log_statement = none,
log_min_error_statement = fatal) so that the tests get to see the
functions only in the backtrace. Although the CF bot is happy with the
attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
there might be some more flakiness to it.Thoughts?
Ran pgperltidy on the new TAP test file added. Please see the attached v2 patch.
I've now moved the new TAP test file to src/test/modules/test_misc/t
as opposed to a new test module to keep it simple. I was not sure why
I hadn't done that in the first place.
Note that backtrace_on_internal_error has been removed, so this patch
will need to be adjusted for that.
I suggest you consider joining forces with thread [0]/messages/by-id/CAGECzQTpdujCEt2SH4DBwRLoDq4HJArGDaxJSsWX0G=tNnzaVA@mail.gmail.com where a
replacement for backtrace_on_internal_error would be discussed. Having
some test coverage for whatever is being developed there might be useful.
[0]: /messages/by-id/CAGECzQTpdujCEt2SH4DBwRLoDq4HJArGDaxJSsWX0G=tNnzaVA@mail.gmail.com
/messages/by-id/CAGECzQTpdujCEt2SH4DBwRLoDq4HJArGDaxJSsWX0G=tNnzaVA@mail.gmail.com