Hello!
On 02.11.2022 21:02, Tom Lane wrote:
So I'm now good with the idea of just not failing. I don't like
the patch as presented though. First, the cfbot is quite rightly
complaining about the "uninitialized variable" warning it draws.
Second, I don't see a good reason to tie the change to logical
replication in any way. Let's just change the Assert to an if(),
as attached.
Fully agreed that is the most optimal solution for that case. Thanks!
Surely it's very rare one but there was a real segfault at production server.
Someone came up with the idea to modify function like public.test_selector()
in repcmd.sql (see above) on the fly with adding to it :last_modification:
field from current time and some other parameters with the help of replace()
inside the creation of the rebuild_test() procedure.
On 03.11.2022 18:29, Tom Lane wrote:
Amit Kapila <amit.kapil...@gmail.com> writes:
LGTM. I don't know if it is a good idea to omit the test case for this
scenario. If required, we can reuse the test case from Sawada-San's
patch in the email [1].
I don't think the cost of that test case is justified by the tiny
probability that it'd ever catch anything. If we were just adding a
query or two to an existing scenario, that could be okay; but spinning
up and syncing a whole new primary and standby database is *expensive*
when you multiply it by the number of times developers and buildfarm
animals are going to run the tests in the future.
There's also the little issue that I'm not sure it would actually
detect a problem if we had one. The case is going to fail, and
what we want to know is just how messily it fails, and I think the
TAP infrastructure isn't very sensitive to that ... especially
if the test isn't even checking for specific error messages.
Seems it is possible to do a test without these remarks. The attached
test uses existing nodes and checks the specific error message. Additionally
i've tried to reduce overall number of nodes previously
used in this test in a similar way.
Would be glad for comments and remarks.
With best wishes,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
commit 722fa6d8c629eb83b3d11ea88b49bab1f700b48d
Author: Anton A. Melnikov <a.melni...@postgrespro.ru>
Date: Mon Nov 14 08:30:26 2022 +0300
Add test for syntax error in the function in a a logical replication
worker and combine some test nodes.
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 6247aa7730..76a6c12cae 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -69,6 +69,9 @@ $node_publisher->wait_for_catchup('sub1');
pass('index predicates do not cause crash');
+$node_publisher->safe_psql('postgres',
+ "CHECKPOINT");
+
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
@@ -81,9 +84,8 @@ $node_subscriber->stop('fast');
# identity set before accepting updates. If it did not it would cause
# an error when an update was attempted.
-$node_publisher = PostgreSQL::Test::Cluster->new('publisher2');
-$node_publisher->init(allows_streaming => 'logical');
-$node_publisher->start;
+$node_publisher->rotate_logfile();
+$node_publisher->start();
$node_publisher->safe_psql('postgres',
"CREATE PUBLICATION pub FOR ALL TABLES");
@@ -102,6 +104,9 @@ is( $node_publisher->psql(
'update to unlogged table without replica identity with FOR ALL TABLES publication'
);
+$node_publisher->safe_psql('postgres',
+ "CHECKPOINT");
+
$node_publisher->stop('fast');
# Bug #16643 - https://postgr.es/m/16643-eaadeb2a1a58d...@postgresql.org
@@ -226,13 +231,13 @@ $node_sub->stop('fast');
# target table's relcache was not being invalidated. This leads to skipping
# UPDATE/DELETE operations during apply on the subscriber side as the columns
# required to search corresponding rows won't get logged.
-$node_publisher = PostgreSQL::Test::Cluster->new('publisher3');
-$node_publisher->init(allows_streaming => 'logical');
-$node_publisher->start;
-$node_subscriber = PostgreSQL::Test::Cluster->new('subscriber3');
-$node_subscriber->init(allows_streaming => 'logical');
-$node_subscriber->start;
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+$node_subscriber->rotate_logfile();
+$node_subscriber->rotate_logfile(); # call twice to synchronize lognames between master and replica
+$node_subscriber->start();
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab_replidentity_index(a int not null, b int not null)");
@@ -296,7 +301,89 @@ is( $node_subscriber->safe_psql(
qq(-1|1),
"update works with REPLICA IDENTITY");
+$node_publisher->safe_psql('postgres',
+ "CHECKPOINT");
+
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# https://www.postgresql.org/message-id/flat/adf0452f-8c6b-7def-d35e-ab516c80088e%40inbox.ru
+
+# The bug was that when a syntax error occurred in a SQL-language or PL/pgSQL-language
+# CREATE FUNCTION or DO command executed in a logical replication worker,
+# we'd suffer a null pointer dereference or assertion failure.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+$node_subscriber->rotate_logfile();
+$node_subscriber->start();
+
+$node_publisher->safe_psql('postgres',
+ 'CREATE TABLE public.test (id int NOT NULL, val integer);');
+
+$node_publisher->safe_psql('postgres',
+ 'CREATE PUBLICATION test_pub FOR TABLE test;');
+
+$node_subscriber->safe_psql('postgres',
+ 'CREATE TABLE test (id int NOT NULL, val integer);');
+
+$node_subscriber->safe_psql('postgres', q{
+ create or replace procedure rebuild_test(
+ ) as
+ $body$
+ declare
+ l_code text:= E'create or replace function public.test_selector(
+ ) returns setof public.test as
+ \$body\$
+ select * from error_name
+ \$body\$ language sql;';
+ begin
+ execute l_code;
+ perform public.test_selector();
+ end
+ $body$ language plpgsql;
+});
+
+$node_subscriber->safe_psql('postgres', '
+ create or replace function test_trg()
+ returns trigger as
+ $body$
+ declare
+ begin
+ call public.rebuild_test();
+ return NULL;
+ end
+ $body$ language plpgsql;
+');
+
+$node_subscriber->safe_psql('postgres',
+ 'create trigger test_trigger after insert or update or delete on test for each row execute function test_trg();');
+
+$node_subscriber->safe_psql('postgres',
+ 'alter table test enable replica trigger test_trigger;');
+
+my $primary_port = $node_publisher->port;
+my $conn_params = "port=" . $primary_port . " dbname=postgres";
+
+my ($stdin, $stdout, $stderr) = $node_subscriber->psql('postgres',
+ qq[CREATE SUBSCRIPTION test_sub CONNECTION \'$conn_params\' PUBLICATION test_pub;]
+);
+
+$node_publisher->wait_for_catchup('test_sub');
+
+ok($stderr =~ /NOTICE: created replication slot \"test_sub\" on publisher/,
+ "ERROR: Subscription on replica or replication slot on publisher weren't created.");
+
+$node_publisher->safe_psql('postgres', q{
+ INSERT INTO test VALUES ('1');
+ });
+
+my $result = $node_subscriber->wait_for_log(
+ "ERROR: relation \"error_name\" does not exist at character"
+);
+
+ok($result,
+ "ERROR: Logical decoding doesn't fail on function error");
+
done_testing();
