On Tue, Jan 21, 2020 at 02:07:30PM +0900, Michael Paquier wrote: > On Mon, Jan 20, 2020 at 11:00:14AM -0800, Andres Freund wrote: >> Hm. I'm think testing this with real LSNs is a better idea. What if the >> node actually already is past FF/FFFFFFFF at this point? Quite unlikely, >> I know, but still. I.e. why not get the current LSN after the INSERT, >> and assert that the slot, after the restart, is that? > > Sure. If not disabling autovacuum in the test, we'd just need to make > sure if that advancing is at least ahead of the INSERT position.
Actually, as the advancing happens only up to this position we just need to make sure that the LSN reported by the slot is the same as the position advanced to. I have switched the test to just do that instead of using a fake LSN. > Anyway, I am still not sure if we should got down the road to just > mark the slot as dirty if any advancing is done and let the follow-up > checkpoint to the work, if the advancing function should do the slot > flush, or if we choose one and make the other an optional choice (not > for back-branches, obviously. Based on my reading of the code, my > guess is that a flush should happen at the end of the advancing > function. I have been chewing on this point for a couple of days, and as we may actually crash between the moment the slot is marked as dirty and the moment the slot information is made consistent, we still have a risk to have the slot go backwards even if the slot information is saved. The window is much narrower, but well, the docs of logical decoding mention that this risk exists. And the patch becomes much more simple without changing the actual behavior present since the feature has been introduced for logical slots. There could be a point in having a new option to flush the slot information, or actually a separate function to flush the slot information, but let's keep that for a future possibility. So attached is an updated patch which addresses the problem just by marking a physical slot as dirty if any advancing is done. Some documentation is added about the fact that an advance is persistent only at the follow-up checkpoint. And the tests are fixed to not use a fake LSN but instead advance to the latest LSN position produced. Any objections? -- Michael
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 7c89694611..e4c4401b68 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -370,6 +370,14 @@ pg_physical_replication_slot_advance(XLogRecPtr moveto)
MyReplicationSlot->data.restart_lsn = moveto;
SpinLockRelease(&MyReplicationSlot->mutex);
retlsn = moveto;
+
+ /*
+ * Dirty the slot so as it is written out at the next checkpoint.
+ * Note that the LSN position advanced may still be lost in the
+ * event of a crash, but this makes the data consistent after a
+ * clean shutdown.
+ */
+ ReplicationSlotMarkDirty();
}
return retlsn;
@@ -467,9 +475,9 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto)
* keep track of their progress, so we should make more of an
* effort to save it for them.
*
- * Dirty the slot so it's written out at the next checkpoint.
- * We'll still lose its position on crash, as documented, but it's
- * better than always losing the position even on clean restart.
+ * Dirty the slot so it is written out at the next checkpoint.
+ * The LSN position advanced to may still be lost on a crash
+ * but this makes the data consistent after a clean shutdown.
*/
ReplicationSlotMarkDirty();
}
@@ -566,15 +574,6 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
values[0] = NameGetDatum(&MyReplicationSlot->data.name);
nulls[0] = false;
- /* Update the on disk state when lsn was updated. */
- if (XLogRecPtrIsInvalid(endlsn))
- {
- ReplicationSlotMarkDirty();
- ReplicationSlotsComputeRequiredXmin(false);
- ReplicationSlotsComputeRequiredLSN();
- ReplicationSlotSave();
- }
-
ReplicationSlotRelease();
/* Return the reached position. */
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 3c743d7d7c..d09ebe65a3 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -3,7 +3,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 32;
+use Test::More tests => 34;
# Initialize master node
my $node_master = get_new_node('master');
@@ -344,3 +344,28 @@ is($catalog_xmin, '',
is($xmin, '', 'xmin of cascaded slot null with hs feedback reset');
is($catalog_xmin, '',
'catalog xmin of cascaded slot still null with hs_feedback reset');
+
+# Test physical slot advancing and its durability. Create a new slot on
+# the primary, not used by any of the standbys. This reserves WAL at creation.
+my $phys_slot = 'phys_slot';
+$node_master->safe_psql('postgres',
+ "SELECT pg_create_physical_replication_slot('$phys_slot', true);");
+$node_master->psql('postgres', "
+ CREATE TABLE tab_phys_slot (a int);
+ INSERT INTO tab_phys_slot VALUES (generate_series(1,10));");
+my $current_lsn = $node_master->safe_psql('postgres',
+ "SELECT pg_current_wal_lsn();");
+chomp($current_lsn);
+my $psql_rc = $node_master->psql('postgres',
+ "SELECT pg_replication_slot_advance('$phys_slot', '$current_lsn'::pg_lsn);");
+is($psql_rc, '0', 'slot advancing with physical slot');
+my $phys_restart_lsn_pre = $node_master->safe_psql('postgres',
+ "SELECT restart_lsn from pg_replication_slots WHERE slot_name = '$phys_slot';");
+chomp($phys_restart_lsn_pre);
+# Slot advance should persist across clean restarts.
+$node_master->restart;
+my $phys_restart_lsn_post = $node_master->safe_psql('postgres',
+ "SELECT restart_lsn from pg_replication_slots WHERE slot_name = '$phys_slot';");
+chomp($phys_restart_lsn_post);
+ok(($phys_restart_lsn_pre cmp $phys_restart_lsn_post) == 0,
+ "physical slot advance persists across restarts");
diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index c23cc4dda7..721d54fc4e 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -7,7 +7,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 10;
+use Test::More tests => 12;
use Config;
# Initialize master node
@@ -135,5 +135,29 @@ is($node_master->psql('postgres', 'DROP DATABASE otherdb'),
is($node_master->slot('otherdb_slot')->{'slot_name'},
undef, 'logical slot was actually dropped with DB');
+# Test logical slot advancing and its durability.
+my $logical_slot = 'logical_slot';
+$node_master->safe_psql('postgres',
+ "SELECT pg_create_logical_replication_slot('$logical_slot', 'test_decoding', false);");
+$node_master->psql('postgres', "
+ CREATE TABLE tab_logical_slot (a int);
+ INSERT INTO tab_logical_slot VALUES (generate_series(1,10));");
+my $current_lsn = $node_master->safe_psql('postgres',
+ "SELECT pg_current_wal_lsn();");
+chomp($current_lsn);
+my $psql_rc = $node_master->psql('postgres',
+ "SELECT pg_replication_slot_advance('$logical_slot', '$current_lsn'::pg_lsn);");
+is($psql_rc, '0', 'slot advancing with logical slot');
+my $logical_restart_lsn_pre = $node_master->safe_psql('postgres',
+ "SELECT restart_lsn from pg_replication_slots WHERE slot_name = '$logical_slot';");
+chomp($logical_restart_lsn_pre);
+# Slot advance should persists across clean restarts.
+$node_master->restart;
+my $logical_restart_lsn_post = $node_master->safe_psql('postgres',
+ "SELECT restart_lsn from pg_replication_slots WHERE slot_name = '$logical_slot';");
+chomp($logical_restart_lsn_post);
+ok(($logical_restart_lsn_pre cmp $logical_restart_lsn_post) == 0,
+ "logical slot advance persists across restarts");
+
# done with the node
$node_master->stop;
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 895b4b7b1b..970d1d145b 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -20470,8 +20470,11 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
<entry>
Advances the current confirmed position of a replication slot named
<parameter>slot_name</parameter>. The slot will not be moved backwards,
- and it will not be moved beyond the current insert location. Returns
- name of the slot and real position to which it was advanced to.
+ and it will not be moved beyond the current insert location. Returns
+ name of the slot and real position to which it was advanced to. The
+ updated slot is marked as dirty if any advancing is done, with its
+ information being written out at the follow-up checkpoint. In the
+ event of a crash, the slot may return to an earlier position.
</entry>
</row>
signature.asc
Description: PGP signature
