From 82c83b337202fa0f5b235bdfaeb992a5cee40ed5 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 1 Apr 2016 16:47:00 -0300 Subject: [PATCH] Fix logical_decoding_timelines test crashes In the test_slot_timelines test module, we were abusing passing NULL values which was received as zeroes in x86, but this breaks in ARM (buildfarm member hamster) by crashing instead. Fix the breakage by marking these functions as STRICT; the InvalidXid value that was previously implicit in NULL values (on x86 at least) can now be passed as 0. Failing to follow the fmgr protocol to check for NULLs beforehand was causing ARM to fail, as evidenced by segmentation faults in buildfarm member hamster. In order to use the new functionality in the test script, use COALESCE in the right spot to avoid forwarding NULL values. This was diagnosed from the hamster crash by Craig Ringer, who also proposed a different patch (checking for NULL values explicitely in the C function code, and keeping the non-strictness in the C functions). I decided to go with this approach instead. --- .../expected/load_extension.out | 2 +- .../test_slot_timelines/sql/load_extension.sql | 2 +- .../test_slot_timelines--1.0.sql | 8 ++++---- .../test_slot_timelines/test_slot_timelines.c | 6 +++--- .../t/006_logical_decoding_timelines.pl | 17 ++++++++++------- 5 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/test/modules/test_slot_timelines/expected/load_extension.out b/src/test/modules/test_slot_timelines/expected/load_extension.out index 14a414aa7e..7c2ad9d7c1 100644 --- a/src/test/modules/test_slot_timelines/expected/load_extension.out +++ b/src/test/modules/test_slot_timelines/expected/load_extension.out @@ -5,7 +5,7 @@ SELECT test_slot_timelines_create_logical_slot('test_slot', 'test_decoding'); (1 row) -SELECT test_slot_timelines_advance_logical_slot('test_slot', txid_current(), txid_current(), pg_current_xlog_location(), pg_current_xlog_location()); +SELECT test_slot_timelines_advance_logical_slot('test_slot', txid_current()::text::xid, txid_current()::text::xid, pg_current_xlog_location(), pg_current_xlog_location()); test_slot_timelines_advance_logical_slot ------------------------------------------ diff --git a/src/test/modules/test_slot_timelines/sql/load_extension.sql b/src/test/modules/test_slot_timelines/sql/load_extension.sql index a71127d471..2440355246 100644 --- a/src/test/modules/test_slot_timelines/sql/load_extension.sql +++ b/src/test/modules/test_slot_timelines/sql/load_extension.sql @@ -2,6 +2,6 @@ CREATE EXTENSION test_slot_timelines; SELECT test_slot_timelines_create_logical_slot('test_slot', 'test_decoding'); -SELECT test_slot_timelines_advance_logical_slot('test_slot', txid_current(), txid_current(), pg_current_xlog_location(), pg_current_xlog_location()); +SELECT test_slot_timelines_advance_logical_slot('test_slot', txid_current()::text::xid, txid_current()::text::xid, pg_current_xlog_location(), pg_current_xlog_location()); SELECT pg_drop_replication_slot('test_slot'); diff --git a/src/test/modules/test_slot_timelines/test_slot_timelines--1.0.sql b/src/test/modules/test_slot_timelines/test_slot_timelines--1.0.sql index 31d7f8ef1c..a1886f732f 100644 --- a/src/test/modules/test_slot_timelines/test_slot_timelines--1.0.sql +++ b/src/test/modules/test_slot_timelines/test_slot_timelines--1.0.sql @@ -3,14 +3,14 @@ CREATE OR REPLACE FUNCTION test_slot_timelines_create_logical_slot(slot_name text, plugin text) RETURNS void -LANGUAGE c AS 'MODULE_PATHNAME'; +STRICT LANGUAGE c AS 'MODULE_PATHNAME'; COMMENT ON FUNCTION test_slot_timelines_create_logical_slot(text, text) IS 'Create a logical slot at a particular lsn and xid. Do not use in production servers, it is not safe. The slot is created with an invalid xmin and lsn.'; -CREATE OR REPLACE FUNCTION test_slot_timelines_advance_logical_slot(slot_name text, new_xmin bigint, new_catalog_xmin bigint, new_restart_lsn pg_lsn, new_confirmed_lsn pg_lsn) +CREATE OR REPLACE FUNCTION test_slot_timelines_advance_logical_slot(slot_name text, new_xmin xid, new_catalog_xmin xid, new_restart_lsn pg_lsn, new_confirmed_lsn pg_lsn) RETURNS void -LANGUAGE c AS 'MODULE_PATHNAME'; +STRICT LANGUAGE c AS 'MODULE_PATHNAME'; -COMMENT ON FUNCTION test_slot_timelines_advance_logical_slot(text, bigint, bigint, pg_lsn, pg_lsn) +COMMENT ON FUNCTION test_slot_timelines_advance_logical_slot(text, xid, xid, pg_lsn, pg_lsn) IS 'Advance a logical slot directly. Do not use this in production servers, it is not safe.'; diff --git a/src/test/modules/test_slot_timelines/test_slot_timelines.c b/src/test/modules/test_slot_timelines/test_slot_timelines.c index 74dd1a041b..1f074881d2 100644 --- a/src/test/modules/test_slot_timelines/test_slot_timelines.c +++ b/src/test/modules/test_slot_timelines/test_slot_timelines.c @@ -85,8 +85,8 @@ Datum test_slot_timelines_advance_logical_slot(PG_FUNCTION_ARGS) { char *slotname = text_to_cstring(PG_GETARG_TEXT_P(0)); - TransactionId new_xmin = (TransactionId) PG_GETARG_INT64(1); - TransactionId new_catalog_xmin = (TransactionId) PG_GETARG_INT64(2); + TransactionId new_xmin = DatumGetTransactionId(PG_GETARG_DATUM(1)); + TransactionId new_catalog_xmin = DatumGetTransactionId(PG_GETARG_DATUM(2)); XLogRecPtr restart_lsn = PG_GETARG_LSN(3); XLogRecPtr confirmed_lsn = PG_GETARG_LSN(4); @@ -95,7 +95,7 @@ test_slot_timelines_advance_logical_slot(PG_FUNCTION_ARGS) ReplicationSlotAcquire(slotname); if (MyReplicationSlot->data.database != MyDatabaseId) - elog(ERROR, "Trying to update a slot on a different database"); + elog(ERROR, "trying to update a slot on a different database"); MyReplicationSlot->data.xmin = new_xmin; MyReplicationSlot->data.catalog_xmin = new_catalog_xmin; diff --git a/src/test/recovery/t/006_logical_decoding_timelines.pl b/src/test/recovery/t/006_logical_decoding_timelines.pl index bc20f405d7..a3a4b6150a 100644 --- a/src/test/recovery/t/006_logical_decoding_timelines.pl +++ b/src/test/recovery/t/006_logical_decoding_timelines.pl @@ -172,8 +172,13 @@ is($stdout, '', 'No slots exist on the replica'); # we're just doing it by hand for this test. This is exposing # postgres innards to SQL so it's unsafe except for testing. $node_master->safe_psql('postgres', 'CREATE EXTENSION test_slot_timelines;'); -my $slotinfo = $node_master->safe_psql('postgres', -'SELECT slot_name, plugin, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn FROM pg_replication_slots ORDER BY slot_name' + +my $slotinfo = $node_master->safe_psql( + 'postgres', + qq{SELECT slot_name, plugin, + COALESCE(xmin, '0'), catalog_xmin, + restart_lsn, confirmed_flush_lsn + FROM pg_replication_slots ORDER BY slot_name} ); diag "Copying slots to replica"; open my $fh, '<', \$slotinfo or die $!; @@ -183,10 +188,7 @@ while (<$fh>) chomp $_; my ($slot_name, $plugin, $xmin, $catalog_xmin, $restart_lsn, $confirmed_flush_lsn) - = map { - if ($_ ne '') { "'$_'" } - else { 'NULL' } - } split qr/\|/, $_; + = map { "'$_'" } split qr/\|/, $_; print "# Copying slot $slot_name,$plugin,$xmin,$catalog_xmin,$restart_lsn,$confirmed_flush_lsn\n"; @@ -208,7 +210,7 @@ is( $stdout, $stdout = $node_replica->safe_psql( 'postgres', - qq{SELECT slot_name, plugin, xmin, catalog_xmin, + qq{SELECT slot_name, plugin, COALESCE(xmin, '0'), catalog_xmin, restart_lsn, confirmed_flush_lsn FROM pg_replication_slots ORDER BY slot_name}); @@ -243,6 +245,7 @@ diag "oldest needed xlog seg is $oldest_needed_segment "; opendir(my $pg_xlog, $node_master->data_dir . "/pg_xlog") or die $!; while (my $seg = readdir $pg_xlog) { + next if $seg eq '.' or $seg eq '..'; next unless $seg >= $oldest_needed_segment && $seg =~ /^[0-9]{24}/; diag "copying xlog seg $seg"; copy( -- 2.40.0