public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Fix timeline-tracking failure while sending a historic timeline 2+ messages / 2 participants [nested] [flat]
* [PATCH] Fix timeline-tracking failure while sending a historic timeline @ 2021-01-05 04:34 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Kyotaro Horiguchi @ 2021-01-05 04:34 UTC (permalink / raw) Walsender should track timeline switches while sending a historic timeline. Regain that behavior, which was broken in PG13, by a thinko of 709d003fbd. Backpatch to PG13. --- src/backend/replication/walsender.c | 2 +- src/test/perl/PostgresNode.pm | 36 ++++++++++++++++++++ src/test/recovery/t/001_stream_rep.pl | 41 ++++++++++++++++++++++- src/test/recovery/t/019_replslot_limit.pl | 37 ++++---------------- 4 files changed, 83 insertions(+), 33 deletions(-) diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 7f87eb7f19..04f6c3ebb4 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -2478,7 +2478,7 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo, XLogSegNo endSegNo; XLByteToSeg(sendTimeLineValidUpto, endSegNo, state->segcxt.ws_segsize); - if (state->seg.ws_segno == endSegNo) + if (nextSegNo == endSegNo) *tli_p = sendTimeLineNextTLI; } diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index 980f1f1533..a08c71b549 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -2138,6 +2138,42 @@ sub pg_recvlogical_upto =pod +=item $node->current_log_position() + +Return the current position of server log. + +=cut + +sub current_log_position +{ + my $self = shift; + + return (stat $self->logfile)[7]; +} + +=pod + +=item $node->find_in_log($pattern, $startpos) + +Returns whether the $pattern occurs after $startpos in the server log. + +=cut + +sub find_in_log +{ + my ($self, $pattern, $startpos) = @_; + + $startpos = 0 unless defined $startpos; + my $log = TestLib::slurp_file($self->logfile); + return 0 if (length($log) <= $startpos); + + $log = substr($log, $startpos); + + return $log =~ m/$pattern/; +} + +=pod + =back =cut diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl index 778f11b28b..8d2b24fe55 100644 --- a/src/test/recovery/t/001_stream_rep.pl +++ b/src/test/recovery/t/001_stream_rep.pl @@ -2,8 +2,9 @@ use strict; use warnings; use PostgresNode; +use Time::HiRes qw(usleep); use TestLib; -use Test::More tests => 36; +use Test::More tests => 37; # Initialize master node my $node_master = get_new_node('master'); @@ -409,3 +410,41 @@ ok( ($phys_restart_lsn_pre cmp $phys_restart_lsn_post) == 0, my $master_data = $node_master->data_dir; ok(!-f "$master_data/pg_wal/$segment_removed", "WAL segment $segment_removed recycled after physical slot advancing"); + +# +# Check if timeline-increment works while reading a historic timeline. +my $node_primary_2 = get_new_node('primary_2'); +# archiving is needed to create .paritial segment +$node_primary_2->init(allows_streaming => 1, has_archiving => 1); +$node_primary_2->start; +$node_primary_2->backup($backup_name); +my $node_standby_3 = get_new_node('standby_3'); +$node_standby_3->init_from_backup($node_primary_2, $backup_name, + has_streaming => 1); +$node_primary_2->stop; +$node_primary_2->set_standby_mode; # increment primary timeline +$node_primary_2->start; +$node_primary_2->promote; +my $logstart = $node_standby_3->current_log_position(); +$node_standby_3->start; + +my $success = 0; +for (my $i = 0 ; $i < 1000; $i++) +{ + if ($node_standby_3->find_in_log( + "requested WAL segment [0-9A-F]+ has already been removed", + $logstart)) + { + last; + } + elsif ($node_standby_3->find_in_log( + "End of WAL reached on timeline", + $logstart)) + { + $success = 1; + last; + } + usleep(100_000); +} + +ok($success, 'Timeline increment while reading a historic timeline'); diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index a7231dcd47..8b3c5de057 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -165,19 +165,17 @@ $node_master->wait_for_catchup($node_standby, 'replay', $start_lsn); $node_standby->stop; -ok( !find_in_log( - $node_standby, - "requested WAL segment [0-9A-F]+ has already been removed"), +ok( !$node_standby->find_in_log( + "requested WAL segment [0-9A-F]+ has already been removed"), 'check that required WAL segments are still available'); # Advance WAL again, the slot loses the oldest segment. -my $logstart = get_log_size($node_master); +my $logstart = $node_master->current_log_position(); advance_wal($node_master, 7); $node_master->safe_psql('postgres', "CHECKPOINT;"); # WARNING should be issued -ok( find_in_log( - $node_master, +ok( $node_master->find_in_log( "invalidating slot \"rep1\" because its restart_lsn [0-9A-F/]+ exceeds max_slot_wal_keep_size", $logstart), 'check that the warning is logged'); @@ -190,14 +188,13 @@ is($result, "rep1|f|t|lost|", 'check that the slot became inactive and the state "lost" persists'); # The standby no longer can connect to the master -$logstart = get_log_size($node_standby); +$logstart = $node_standby->current_log_position(); $node_standby->start; my $failed = 0; for (my $i = 0; $i < 10000; $i++) { - if (find_in_log( - $node_standby, + if ($node_standby->find_in_log( "requested WAL segment [0-9A-F]+ has already been removed", $logstart)) { @@ -264,25 +261,3 @@ sub advance_wal } return; } - -# return the size of logfile of $node in bytes -sub get_log_size -{ - my ($node) = @_; - - return (stat $node->logfile)[7]; -} - -# find $pat in logfile of $node after $off-th byte -sub find_in_log -{ - my ($node, $pat, $off) = @_; - - $off = 0 unless defined $off; - my $log = TestLib::slurp_file($node->logfile); - return 0 if (length($log) <= $off); - - $log = substr($log, $off); - - return $log =~ m/$pat/; -} -- 2.27.0 ----Next_Part(Thu_Jan__7_16_32_36_2021_122)---- ^ permalink raw reply [nested|flat] 2+ messages in thread
* [PATCH v3] Don't lose column values on REPACK @ 2026-05-04 15:19 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Álvaro Herrera @ 2026-05-04 15:19 UTC (permalink / raw) Commit 28d534e2ae0a introduced reform_tuple() with a fast path that returns the source tuple verbatim when no dropped columns require fixing up. I (Álvaro) failed to realize that this broke handling of columns with a 'missingval' defined: after a VACUUM FULL, CLUSTER, or REPACK operation, the catalogued missingval is thrown away, so the tuples are no longer correct. Fix by forcing the rewrite when the tuple is shorter than the tuple descriptor. Author: Satya Narlapuram <[email protected]> Discussion: https://postgr.es/m/CAHg+QDeoccU5CudrJpmSKZfKZ1gRMNY=5BxSC=JpHgkonzgcOw@mail.gmail.com --- contrib/test_decoding/expected/repack.out | 22 +++++++++++++ contrib/test_decoding/sql/repack.sql | 9 +++++ src/backend/access/heap/heapam_handler.c | 38 +++++++++++++++++----- src/test/regress/expected/fast_default.out | 36 ++++++++++++++++++++ src/test/regress/sql/fast_default.sql | 16 +++++++++ 5 files changed, 112 insertions(+), 9 deletions(-) diff --git a/contrib/test_decoding/expected/repack.out b/contrib/test_decoding/expected/repack.out index 1f99f26c1f8..cf93c1f0d3d 100644 --- a/contrib/test_decoding/expected/repack.out +++ b/contrib/test_decoding/expected/repack.out @@ -29,3 +29,25 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a DROP TABLE ptnowner; DROP ROLE regress_ptnowner; +-- Verify that REPACK (CONCURRENTLY) doesn't lose "attmissingval" columns +CREATE TABLE rpk_missing (id int PRIMARY KEY); +INSERT INTO rpk_missing SELECT generate_series(1, 3); +ALTER TABLE rpk_missing ADD COLUMN a int DEFAULT 42; +SELECT * FROM rpk_missing; + id | a +----+---- + 1 | 42 + 2 | 42 + 3 | 42 +(3 rows) + +REPACK (CONCURRENTLY) rpk_missing; +SELECT * FROM rpk_missing; + id | a +----+---- + 1 | 42 + 2 | 42 + 3 | 42 +(3 rows) + +DROP TABLE rpk_missing; diff --git a/contrib/test_decoding/sql/repack.sql b/contrib/test_decoding/sql/repack.sql index c3bfae61f7f..6164dd4235f 100644 --- a/contrib/test_decoding/sql/repack.sql +++ b/contrib/test_decoding/sql/repack.sql @@ -23,3 +23,12 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; DROP TABLE ptnowner; DROP ROLE regress_ptnowner; + +-- Verify that REPACK (CONCURRENTLY) doesn't lose "attmissingval" columns +CREATE TABLE rpk_missing (id int PRIMARY KEY); +INSERT INTO rpk_missing SELECT generate_series(1, 3); +ALTER TABLE rpk_missing ADD COLUMN a int DEFAULT 42; +SELECT * FROM rpk_missing; +REPACK (CONCURRENTLY) rpk_missing; +SELECT * FROM rpk_missing; +DROP TABLE rpk_missing; diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 20d3b46e062..2268cc277bc 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2396,10 +2396,10 @@ heap_insert_for_repack(HeapTuple tuple, Relation OldHeap, Relation NewHeap, /* * Subroutine for reform_and_rewrite_tuple and heap_insert_for_repack. * - * Deform the given tuple, set values of dropped columns to NULL, form a new - * tuple and return it. If no attributes need to be changed in this way, a - * copy of the original tuple is returned. Caller is responsible for freeing - * the returned tuple. + * Deform the given tuple, set values of dropped columns to NULL, and fill in + * any values from attmissingval; then form a new tuple and return it. If no + * attributes need to be changed, a copy of the original tuple is returned. + * Caller is responsible for freeing the returned tuple. * * XXX this coding assumes that both relations have the same tupledesc. */ @@ -2411,13 +2411,33 @@ reform_tuple(HeapTuple tuple, Relation OldHeap, Relation NewHeap, TupleDesc newTupDesc = RelationGetDescr(NewHeap); bool needs_reform = false; - /* Skip work if the tuple doesn't need any attributes changed */ - for (int i = 0; i < newTupDesc->natts; i++) + /* + * A short tuple might require values from attmissing val, so activate the + * coding unconditionally in that case. The value might legitimally be + * NULL otherwise, so this is slightly wasteful, but it probably beats + * having to test each attribute for presence of attmissingval each time. + */ + if (HeapTupleHeaderGetNatts(tuple->t_data) < newTupDesc->natts) + needs_reform = true; + + /* + * If the column has been dropped but a value is still present, we can + * optimize storage now by getting rid of it. + */ + if (!needs_reform) { - if (TupleDescCompactAttr(newTupDesc, i)->attisdropped && - !heap_attisnull(tuple, i + 1, newTupDesc)) - needs_reform = true; + for (int i = 0; i < newTupDesc->natts; i++) + { + if (TupleDescCompactAttr(newTupDesc, i)->attisdropped && + !heap_attisnull(tuple, i + 1, newTupDesc)) + { + needs_reform = true; + break; + } + } } + + /* Skip work if no changes are needed */ if (!needs_reform) return heap_copytuple(tuple); diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out index bd142ed481c..5813f1c61a5 100644 --- a/src/test/regress/expected/fast_default.out +++ b/src/test/regress/expected/fast_default.out @@ -969,6 +969,42 @@ SELECT count(*) 0 (1 row) +-- Verify that table-rewriting maintenance commands preserve attmissingval +-- columns. +CREATE TABLE t (id int PRIMARY KEY); +INSERT INTO t SELECT generate_series(1, 3); +ALTER TABLE t ADD COLUMN a int DEFAULT 42; +ALTER TABLE t ADD COLUMN b int NOT NULL DEFAULT 7 CHECK (b > 0); +VACUUM FULL t; +SELECT * FROM t ORDER BY id; + id | a | b +----+----+--- + 1 | 42 | 7 + 2 | 42 | 7 + 3 | 42 | 7 +(3 rows) + +ALTER TABLE t ADD COLUMN c text DEFAULT 'hello'; +CLUSTER t USING t_pkey; +SELECT * FROM t ORDER BY id; + id | a | b | c +----+----+---+------- + 1 | 42 | 7 | hello + 2 | 42 | 7 | hello + 3 | 42 | 7 | hello +(3 rows) + +ALTER TABLE t ADD COLUMN d int DEFAULT 99; +REPACK t; +SELECT * FROM t ORDER BY id; + id | a | b | c | d +----+----+---+-------+---- + 1 | 42 | 7 | hello | 99 + 2 | 42 | 7 | hello | 99 + 3 | 42 | 7 | hello | 99 +(3 rows) + +DROP TABLE t; -- cleanup DROP FOREIGN TABLE ft1; DROP SERVER s0; diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql index 8b31d317d73..e5d9a3d2fd1 100644 --- a/src/test/regress/sql/fast_default.sql +++ b/src/test/regress/sql/fast_default.sql @@ -653,6 +653,22 @@ SELECT count(*) WHERE attrelid = 'ft1'::regclass AND (attmissingval IS NOT NULL OR atthasmissing); +-- Verify that table-rewriting maintenance commands preserve attmissingval +-- columns. +CREATE TABLE t (id int PRIMARY KEY); +INSERT INTO t SELECT generate_series(1, 3); +ALTER TABLE t ADD COLUMN a int DEFAULT 42; +ALTER TABLE t ADD COLUMN b int NOT NULL DEFAULT 7 CHECK (b > 0); +VACUUM FULL t; +SELECT * FROM t ORDER BY id; +ALTER TABLE t ADD COLUMN c text DEFAULT 'hello'; +CLUSTER t USING t_pkey; +SELECT * FROM t ORDER BY id; +ALTER TABLE t ADD COLUMN d int DEFAULT 99; +REPACK t; +SELECT * FROM t ORDER BY id; +DROP TABLE t; + -- cleanup DROP FOREIGN TABLE ft1; DROP SERVER s0; -- 2.47.3 --ouwjmuj5jnxenbix-- ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-05-04 15:19 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-05 04:34 [PATCH] Fix timeline-tracking failure while sending a historic timeline Kyotaro Horiguchi <[email protected]> 2026-05-04 15:19 [PATCH v3] Don't lose column values on REPACK Álvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox