Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit e2ed7e3

Browse files
committed
Fix GetStrictOldestNonRemovableTransactionId() on standby
e85662d implemented GetStrictOldestNonRemovableTransactionId() function for computation of xid horizon that avoid reporting of false errors. However, GetStrictOldestNonRemovableTransactionId() uses GetRunningTransactionData() even on standby leading to an assertion failure. Given that we decided to ignore KnownAssignedXids and standby can't have own running xids, we switch to use TransamVariables->nextXid as a xid horizon. Also, revise the comment regarding ignoring KnownAssignedXids with more detailed reasoning provided by Heikki. Reported-by: Heikki Linnakangas Discussion: https://postgr.es/m/42218c4f-2c8d-40a3-8743-4d34dd0e4cce%40iki.fi Reviewed-by: Heikki Linnakangas
1 parent 9e9a2b7 commit e2ed7e3

File tree

2 files changed

+40
-5
lines changed

2 files changed

+40
-5
lines changed

contrib/pg_visibility/pg_visibility.c

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -546,11 +546,21 @@ collect_visibility_data(Oid relid, bool include_pd)
546546
*
547547
* 1. Ignore processes xmin's, because they consider connection to other
548548
* databases that were ignored before.
549-
* 2. Ignore KnownAssignedXids, because they are not database-aware. At the
550-
* same time, the primary could compute its horizons database-aware.
549+
* 2. Ignore KnownAssignedXids, as they are not database-aware. Although we
550+
* now perform minimal checking on a standby by always using nextXid, this
551+
* approach is better than nothing and will at least catch extremely broken
552+
* cases where a xid is in the future.
551553
* 3. Ignore walsender xmin, because it could go backward if some replication
552554
* connections don't use replication slots.
553555
*
556+
* While it might seem like we could use KnownAssignedXids for shared
557+
* catalogs, since shared catalogs rely on a global horizon rather than a
558+
* database-specific one - there are potential edge cases. For example, a
559+
* transaction may crash on the primary without writing a commit/abort record.
560+
* This would lead to a situation where it appears to still be running on the
561+
* standby, even though it has already ended on the primary. For this reason,
562+
* it's safer to ignore KnownAssignedXids, even for shared catalogs.
563+
*
554564
* As a result, we're using only currently running xids to compute the horizon.
555565
* Surely these would significantly sacrifice accuracy. But we have to do so
556566
* to avoid reporting false errors.
@@ -560,7 +570,17 @@ GetStrictOldestNonRemovableTransactionId(Relation rel)
560570
{
561571
RunningTransactions runningTransactions;
562572

563-
if (rel == NULL || rel->rd_rel->relisshared || RecoveryInProgress())
573+
if (RecoveryInProgress())
574+
{
575+
TransactionId result;
576+
577+
/* As we ignore KnownAssignedXids on standby, just pick nextXid */
578+
LWLockAcquire(XidGenLock, LW_SHARED);
579+
result = XidFromFullTransactionId(TransamVariables->nextXid);
580+
LWLockRelease(XidGenLock);
581+
return result;
582+
}
583+
else if (rel == NULL || rel->rd_rel->relisshared)
564584
{
565585
/* Shared relation: take into account all running xids */
566586
runningTransactions = GetRunningTransactionData();

contrib/pg_visibility/t/001_concurrent_transaction.pl

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,18 @@
1010
use Test::More;
1111

1212

13+
# Initialize the primary node
1314
my $node = PostgreSQL::Test::Cluster->new('main');
14-
15-
$node->init;
15+
$node->init(allows_streaming => 1);
1616
$node->start;
1717

18+
# Initialize the streaming standby
19+
my $backup_name = 'my_backup';
20+
$node->backup($backup_name);
21+
my $standby = PostgreSQL::Test::Cluster->new('standby');
22+
$standby->init_from_backup($node, $backup_name, has_streaming => 1);
23+
$standby->start;
24+
1825
# Setup another database
1926
$node->safe_psql("postgres", "CREATE DATABASE other_database;\n");
2027
my $bsession = $node->background_psql('other_database');
@@ -39,9 +46,17 @@
3946
# There should be no false negatives
4047
ok($result eq "", "pg_check_visible() detects no errors");
4148

49+
# Run pg_check_visible() on standby
50+
$result = $standby->safe_psql("postgres",
51+
"SELECT * FROM pg_check_visible('vacuum_test');");
52+
53+
# There should be no false negatives either
54+
ok($result eq "", "pg_check_visible() detects no errors");
55+
4256
# Shutdown
4357
$bsession->query_safe("COMMIT;");
4458
$bsession->quit;
4559
$node->stop;
60+
$standby->stop;
4661

4762
done_testing();

0 commit comments

Comments
 (0)