public inbox for [email protected]help / color / mirror / Atom feed
OCD questions about LWTRANCHE_REPLICATION_ORIGIN 3+ messages / 3 participants [nested] [flat]
* OCD questions about LWTRANCHE_REPLICATION_ORIGIN @ 2018-02-12 01:35 Thomas Munro <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Thomas Munro @ 2018-02-12 01:35 UTC (permalink / raw) To: pgsql-hackers Hi, Just curious about a harmless inconsistency, really: why does src/backend/replication/logical/origin.c bother to copy LWTRANCHE_REPLICATION_ORIGIN into shm and then LWLockRegisterTranche() in every process from the shm copy? I guess because it used to allocate the tranche ID dynamically with LWLockNewTrancheId(), but it doesn't anymore. So my questions are: 1. Why shouldn't we just call LWLockRegisterTranche(LWTRANCHE_REPLICATION_ORIGIN, "replication_origin") and get rid of "tranche_id" in shm? 2. What is our basis for registering some built-in LWLock tranches in XXXShmemInit() and the rest in RegisterLWLockTranches()? Why shouldn't we just move this one over to RegisterLWLockTranches()? Then it'd be easier to verify that we've registered them all. Come to think of it, I suppose someone might even argue that we should have lwtranchenames.txt, much like lwlocknames.txt, that would produce the enumerators and register the display name for all built-in tranches. And then a hypothetical cross-check script that runs under check-world as previously mooted could verify that every display name mentioned in there is also mentioned in monitoring.sgml. I say that as someone who recently screwed up and forgot to register the display name of a new built-in tranche[1]... [1] https://www.postgresql.org/message-id/CAEepm%3D1WM%3D02cKT7hinnx8xRhN2QdF%2BWfE6gk5oMPorwLi%2BXiA%40... -- Thomas Munro http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH v18] Make End-Of-Recovery error less scary @ 2020-02-28 06:52 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Kyotaro Horiguchi @ 2020-02-28 06:52 UTC (permalink / raw) When recovery in any type ends, we see a bit scary error message like "invalid record length" that suggests something serious is happening. Actually if recovery meets a record with length = 0, that usually means it finished applying all available WAL records. Make this message less scary as "reached end of WAL". Instead, raise the error level for other kind of WAL failure to WARNING. --- src/backend/access/transam/xlogreader.c | 145 +++++++++++++++++----- src/backend/access/transam/xlogrecovery.c | 92 ++++++++++---- src/backend/replication/walreceiver.c | 7 +- src/bin/pg_waldump/pg_waldump.c | 13 +- src/include/access/xlogreader.h | 1 + src/test/recovery/t/011_crash_recovery.pl | 106 ++++++++++++++++ 6 files changed, 306 insertions(+), 58 deletions(-) diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index e437c42992..0942265408 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -46,6 +46,8 @@ static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen); static void XLogReaderInvalReadState(XLogReaderState *state); static XLogPageReadResult XLogDecodeNextRecord(XLogReaderState *state, bool non_blocking); +static bool ValidXLogRecordLength(XLogReaderState *state, XLogRecPtr RecPtr, + XLogRecord *record); static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr, XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess); static bool ValidXLogRecord(XLogReaderState *state, XLogRecord *record, @@ -147,6 +149,7 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir, pfree(state); return NULL; } + state->EndOfWAL = false; state->errormsg_buf[0] = '\0'; /* @@ -552,6 +555,7 @@ XLogDecodeNextRecord(XLogReaderState *state, bool nonblocking) /* reset error state */ state->errormsg_buf[0] = '\0'; decoded = NULL; + state->EndOfWAL = false; state->abortedRecPtr = InvalidXLogRecPtr; state->missingContrecPtr = InvalidXLogRecPtr; @@ -633,25 +637,21 @@ restart: Assert(pageHeaderSize <= readOff); /* - * Read the record length. + * Validate the record header. * - * NB: Even though we use an XLogRecord pointer here, the whole record - * header might not fit on this page. xl_tot_len is the first field of the - * struct, so it must be on this page (the records are MAXALIGNed), but we - * cannot access any other fields until we've verified that we got the - * whole header. - */ - record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ); - total_len = record->xl_tot_len; - - /* - * If the whole record header is on this page, validate it immediately. - * Otherwise do just a basic sanity check on xl_tot_len, and validate the - * rest of the header after reading it from the next page. The xl_tot_len + * Even though we use an XLogRecord pointer here, the whole record header + * might not fit on this page. If the whole record header is on this page, + * validate it immediately. Even otherwise xl_tot_len must be on this page + * (it is the first field of MAXALIGNed records), but we still cannot + * access any further fields until we've verified that we got the whole + * header, so do just a basic sanity check on record length, and validate + * the rest of the header after reading it from the next page. The length * check is necessary here to ensure that we enter the "Need to reassemble * record" code path below; otherwise we might fail to apply * ValidXLogRecordHeader at all. */ + record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ); + if (targetRecOff <= XLOG_BLCKSZ - SizeOfXLogRecord) { if (!ValidXLogRecordHeader(state, RecPtr, state->DecodeRecPtr, record, @@ -661,18 +661,14 @@ restart: } else { - /* XXX: more validation should be done here */ - if (total_len < SizeOfXLogRecord) - { - report_invalid_record(state, - "invalid record length at %X/%X: wanted %u, got %u", - LSN_FORMAT_ARGS(RecPtr), - (uint32) SizeOfXLogRecord, total_len); + if (!ValidXLogRecordLength(state, RecPtr, record)) goto err; - } + gotheader = false; } + total_len = record->xl_tot_len; + /* * Find space to decode this record. Don't allow oversized allocation if * the caller requested nonblocking. Otherwise, we *have* to try to @@ -904,6 +900,15 @@ err: */ state->abortedRecPtr = RecPtr; state->missingContrecPtr = targetPagePtr; + + /* + * If the message is not set yet, that means we failed to load the + * page for the record. Otherwise do not hide the existing message. + */ + if (state->errormsg_buf[0] == '\0') + report_invalid_record(state, + "missing contrecord at %X/%X", + LSN_FORMAT_ARGS(RecPtr)); } if (decoded && decoded->oversized) @@ -1083,6 +1088,60 @@ XLogReaderInvalReadState(XLogReaderState *state) state->readLen = 0; } +/* + * Validate record length of an XLOG record header. + * + * This is substantially a part of ValidXLogRecordHeader. But XLogReadRecord + * needs this separate from the function in case of a partial record header. + */ +static bool +ValidXLogRecordLength(XLogReaderState *state, XLogRecPtr RecPtr, + XLogRecord *record) +{ + if (record->xl_tot_len == 0) + { + char *p; + char *pe; + + /* + * We are almost sure reaching the end of WAL, make sure that the + * whole page after the record is filled with zeroes. + */ + p = (char *) record; + pe = p + XLOG_BLCKSZ - (RecPtr & (XLOG_BLCKSZ - 1)); + + while (*p == 0 && p < pe) + p++; + + if (p == pe) + { + /* + * The page after the record is completely zeroed. That suggests + * we don't have a record after this point. We don't bother + * checking the pages after since they are not zeroed in the case + * of recycled segments. + */ + report_invalid_record(state, "empty record at %X/%X", + LSN_FORMAT_ARGS(RecPtr)); + + /* notify end-of-wal to callers */ + state->EndOfWAL = true; + return false; + } + } + + if (record->xl_tot_len < SizeOfXLogRecord) + { + report_invalid_record(state, + "invalid record length at %X/%X: wanted %u, got %u", + LSN_FORMAT_ARGS(RecPtr), + (uint32) SizeOfXLogRecord, record->xl_tot_len); + return false; + } + + return true; +} + /* * Validate an XLOG record header. * @@ -1094,14 +1153,9 @@ ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr, XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess) { - if (record->xl_tot_len < SizeOfXLogRecord) - { - report_invalid_record(state, - "invalid record length at %X/%X: wanted %u, got %u", - LSN_FORMAT_ARGS(RecPtr), - (uint32) SizeOfXLogRecord, record->xl_tot_len); + if (!ValidXLogRecordLength(state, RecPtr, record)) return false; - } + if (record->xl_rmid > RM_MAX_ID) { report_invalid_record(state, @@ -1200,6 +1254,31 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr, XLogSegNoOffsetToRecPtr(segno, offset, state->segcxt.ws_segsize, recaddr); + StaticAssertStmt(XLOG_PAGE_MAGIC != 0, "XLOG_PAGE_MAGIC is zero"); + + if (hdr->xlp_magic == 0) + { + /* Regard an empty page as End-Of-WAL */ + int i; + + for (i = 0; i < XLOG_BLCKSZ && phdr[i] == 0; i++); + if (i == XLOG_BLCKSZ) + { + char fname[MAXFNAMELEN]; + + XLogFileName(fname, state->seg.ws_tli, segno, + state->segcxt.ws_segsize); + + report_invalid_record(state, + "empty page in log segment %s, offset %u", + fname, + offset); + state->EndOfWAL = true; + return false; + } + + /* The same condition will be caught as invalid magic number */ + } if (hdr->xlp_magic != XLOG_PAGE_MAGIC) { char fname[MAXFNAMELEN]; @@ -1285,6 +1364,14 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr, LSN_FORMAT_ARGS(hdr->xlp_pageaddr), fname, offset); + + /* + * If the page address is less than expected we assume it is an unused + * page in a recycled segment. + */ + if (hdr->xlp_pageaddr < recaddr) + state->EndOfWAL = true; + return false; } diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index 8b22c4e634..de8be3b834 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -1592,7 +1592,7 @@ PerformWalRecovery(void) /* just have to read next record after CheckPoint */ Assert(xlogreader->ReadRecPtr == CheckPointLoc); replayTLI = CheckPointTLI; - record = ReadRecord(xlogreader, LOG, false, replayTLI); + record = ReadRecord(xlogreader, WARNING, false, replayTLI); } if (record != NULL) @@ -1706,7 +1706,7 @@ PerformWalRecovery(void) } /* Else, try to fetch the next WAL record */ - record = ReadRecord(xlogreader, LOG, false, replayTLI); + record = ReadRecord(xlogreader, WARNING, false, replayTLI); } while (record != NULL); /* @@ -1765,13 +1765,20 @@ PerformWalRecovery(void) InRedo = false; } - else + else if (xlogreader->EndOfWAL) { /* there are no WAL records following the checkpoint */ ereport(LOG, (errmsg("redo is not required"))); } + else + { + /* broken record found */ + ereport(WARNING, + (errmsg("redo is skipped"), + errhint("This suggests WAL file corruption. You might need to check the database."))); + } /* * This check is intentionally after the above log messages that indicate @@ -2949,6 +2956,7 @@ ReadRecord(XLogReaderState *xlogreader, int emode, for (;;) { char *errormsg; + XLogRecPtr ErrRecPtr = InvalidXLogRecPtr; record = XLogReadRecord(xlogreader, &errormsg); if (record == NULL) @@ -2964,6 +2972,18 @@ ReadRecord(XLogReaderState *xlogreader, int emode, { abortedRecPtr = xlogreader->abortedRecPtr; missingContrecPtr = xlogreader->missingContrecPtr; + ErrRecPtr = abortedRecPtr; + } + else + { + /* + * EndRecPtr is the LSN we tried to read but failed. In the + * case of decoding error, it is at the end of the failed + * record but we don't have a means for now to know EndRecPtr + * is pointing to which of the beginning or ending of the + * failed record. + */ + ErrRecPtr = xlogreader->EndRecPtr; } if (readFile >= 0) @@ -2973,13 +2993,16 @@ ReadRecord(XLogReaderState *xlogreader, int emode, } /* - * We only end up here without a message when XLogPageRead() - * failed - in that case we already logged something. In - * StandbyMode that only happens if we have been triggered, so we - * shouldn't loop anymore in that case. + * We only end up here without a message when XLogPageRead() failed + * in that case we already logged something, or just met end-of-WAL + * conditions. In StandbyMode that only happens if we have been + * triggered, so we shouldn't loop anymore in that case. When + * EndOfWAL is true, we don't emit that error if any immediately + * and instead will show it as a part of a decent end-of-wal + * message later. */ - if (errormsg) - ereport(emode_for_corrupt_record(emode, xlogreader->EndRecPtr), + if (!xlogreader->EndOfWAL && errormsg) + ereport(emode_for_corrupt_record(emode, ErrRecPtr), (errmsg_internal("%s", errormsg) /* already translated */ )); } @@ -3010,11 +3033,14 @@ ReadRecord(XLogReaderState *xlogreader, int emode, /* Great, got a record */ return record; } - else + + Assert(ErrRecPtr != InvalidXLogRecPtr); + + /* No valid record available from this source */ + lastSourceFailed = true; + + if (!fetching_ckpt) { - /* No valid record available from this source */ - lastSourceFailed = true; - /* * If archive recovery was requested, but we were still doing * crash recovery, switch to archive recovery and retry using the @@ -3027,11 +3053,16 @@ ReadRecord(XLogReaderState *xlogreader, int emode, * we'd have no idea how far we'd have to replay to reach * consistency. So err on the safe side and give up. */ - if (!InArchiveRecovery && ArchiveRecoveryRequested && - !fetching_ckpt) + if (!InArchiveRecovery && ArchiveRecoveryRequested) { + /* + * We don't report this as LOG, since we don't stop recovery + * here + */ ereport(DEBUG1, - (errmsg_internal("reached end of WAL in pg_wal, entering archive recovery"))); + (errmsg_internal("reached end of WAL at %X/%X on timeline %u in pg_wal during crash recovery, entering archive recovery", + LSN_FORMAT_ARGS(ErrRecPtr), + replayTLI))); InArchiveRecovery = true; if (StandbyModeRequested) StandbyMode = true; @@ -3052,12 +3083,24 @@ ReadRecord(XLogReaderState *xlogreader, int emode, continue; } - /* In standby mode, loop back to retry. Otherwise, give up. */ - if (StandbyMode && !CheckForStandbyTrigger()) - continue; - else - return NULL; + /* + * recovery ended. + * + * Emit a decent message if we met end-of-WAL. Otherwise we should + * have already emitted an error message. + */ + if (xlogreader->EndOfWAL) + ereport(LOG, + (errmsg("reached end of WAL at %X/%X on timeline %u", + LSN_FORMAT_ARGS(ErrRecPtr), replayTLI), + (errormsg ? errdetail_internal("%s", errormsg) : 0))); } + + /* In standby mode, loop back to retry. Otherwise, give up. */ + if (StandbyMode && !CheckForStandbyTrigger()) + continue; + else + return NULL; } } @@ -3139,12 +3182,16 @@ retry: private->replayTLI, xlogreader->EndRecPtr)) { + Assert(!StandbyMode || CheckForStandbyTrigger()); + if (readFile >= 0) close(readFile); readFile = -1; readLen = 0; readSource = XLOG_FROM_ANY; + /* promotion exit is not end-of-WAL */ + xlogreader->EndOfWAL = !StandbyMode; return -1; } } @@ -3777,7 +3824,8 @@ emode_for_corrupt_record(int emode, XLogRecPtr RecPtr) { static XLogRecPtr lastComplaint = 0; - if (readSource == XLOG_FROM_PG_WAL && emode == LOG) + /* use currentSource as readSource is reset at failure */ + if (currentSource == XLOG_FROM_PG_WAL && emode <= WARNING) { if (RecPtr == lastComplaint) emode = DEBUG1; diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index 3c9411e221..2f9ef9bf31 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -472,10 +472,9 @@ WalReceiverMain(void) else if (len < 0) { ereport(LOG, - (errmsg("replication terminated by primary server"), - errdetail("End of WAL reached on timeline %u at %X/%X.", - startpointTLI, - LSN_FORMAT_ARGS(LogstreamResult.Write)))); + (errmsg("replication terminated by primary server at %X/%X on timeline %u.", + LSN_FORMAT_ARGS(LogstreamResult.Write), + startpointTLI))); endofwal = true; break; } diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 4cb40d068a..2a78e954de 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -1310,9 +1310,16 @@ main(int argc, char **argv) exit(0); if (errormsg) - fatal_error("error in WAL record at %X/%X: %s", - LSN_FORMAT_ARGS(xlogreader_state->ReadRecPtr), - errormsg); + { + if (xlogreader_state->EndOfWAL) + pg_log_info("end of WAL at %X/%X: %s", + LSN_FORMAT_ARGS(xlogreader_state->EndRecPtr), + errormsg); + else + fatal_error("error in WAL record at %X/%X: %s", + LSN_FORMAT_ARGS(xlogreader_state->EndRecPtr), + errormsg); + } XLogReaderFree(xlogreader_state); diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index f4388cc9be..21a8f9552c 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -201,6 +201,7 @@ struct XLogReaderState */ XLogRecPtr ReadRecPtr; /* start of last record read */ XLogRecPtr EndRecPtr; /* end+1 of last record read */ + bool EndOfWAL; /* was the last attempt EOW? */ /* * Set at the end of recovery: the start point of a partial record at the diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.pl index 1b57d01046..f8b4a8417c 100644 --- a/src/test/recovery/t/011_crash_recovery.pl +++ b/src/test/recovery/t/011_crash_recovery.pl @@ -9,7 +9,9 @@ use warnings; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; +use IPC::Run; +my $reached_eow_pat = "reached end of WAL at "; my $node = PostgreSQL::Test::Cluster->new('primary'); $node->init(allows_streaming => 1); $node->start; @@ -47,7 +49,15 @@ is($node->safe_psql('postgres', qq[SELECT pg_xact_status('$xid');]), # Crash and restart the postmaster $node->stop('immediate'); +my $logstart = get_log_size($node); $node->start; +my $max_attempts = 360; +while ($max_attempts-- >= 0) +{ + last if (find_in_log($node, $reached_eow_pat, $logstart)); + sleep 0.5; +} +ok ($max_attempts >= 0, "end-of-wal is logged"); # Make sure we really got a new xid cmp_ok($node->safe_psql('postgres', 'SELECT pg_current_xact_id()'), @@ -60,4 +70,100 @@ is($node->safe_psql('postgres', qq[SELECT pg_xact_status('$xid');]), $stdin .= "\\q\n"; $tx->finish; # wait for psql to quit gracefully +my $segsize = $node->safe_psql('postgres', + qq[SELECT setting FROM pg_settings WHERE name = 'wal_segment_size';]); + +# make sure no records afterwards go to the next segment +$node->safe_psql('postgres', qq[ + SELECT pg_switch_wal(); + CHECKPOINT; + CREATE TABLE t(); +]); +$node->stop('immediate'); + +# identify REDO WAL file +my $cmd = "pg_controldata -D " . $node->data_dir(); +$cmd = ['pg_controldata', '-D', $node->data_dir()]; +$stdout = ''; +$stderr = ''; +IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr; +ok($stdout =~ /^Latest checkpoint's REDO WAL file:[ \t] *(.+)$/m, + "checkpoint file is identified"); +my $chkptfile = $1; + +# identify the last record +my $walfile = $node->data_dir() . "/pg_wal/$chkptfile"; +$cmd = ['pg_waldump', $walfile]; +$stdout = ''; +$stderr = ''; +my $lastrec; +IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr; +foreach my $l (split(/\r?\n/, $stdout)) +{ + $lastrec = $l; +} +ok(defined $lastrec, "last WAL record is extracted"); +ok($stderr =~ /end of WAL at ([0-9A-F\/]+): .* at \g1/, + "pg_waldump emits the correct ending message"); + +# read the last record LSN excluding leading zeroes +ok ($lastrec =~ /, lsn: 0\/0*([1-9A-F][0-9A-F]+),/, + "LSN of the last record identified"); +my $lastlsn = $1; + +# corrupt the last record +my $offset = hex($lastlsn) % $segsize; +open(my $segf, '+<', $walfile) or die "failed to open $walfile\n"; +seek($segf, $offset, 0); # halfway break the last record +print $segf "\0\0\0\0"; +close($segf); + +# pg_waldump complains about the corrupted record +$stdout = ''; +$stderr = ''; +IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr; +ok($stderr =~ /fatal: error in WAL record at 0\/$lastlsn: .* at 0\/$lastlsn/, + "pg_waldump emits the correct error message"); + +# also server complains +$logstart = get_log_size($node); +$node->start; +$max_attempts = 360; +while ($max_attempts-- >= 0) +{ + last if (find_in_log($node, "WARNING: invalid record length at 0/$lastlsn: wanted [0-9]+, got 0", + $logstart)); + sleep 0.5; +} +ok($max_attempts >= 0, "header error is logged at $lastlsn"); + +# no end-of-wal message should be seen this time +ok(!find_in_log($node, $reached_eow_pat, $logstart), + "false log message is not emitted"); + +$node->stop('immediate'); + done_testing(); + +#### helper routines +# 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 = PostgreSQL::Test::Utils::slurp_file($node->logfile); + return 0 if (length($log) <= $off); + + $log = substr($log, $off); + + return $log =~ m/$pat/; +} -- 2.27.0 ----Next_Part(Tue_Mar_29_15_07_01_2022_552)---- ^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH v2] Fix duplicate arbiter detection during REINDEX CONCURRENTLY on partitions @ 2026-01-25 17:01 Mikhail Nikalayeu <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Mikhail Nikalayeu @ 2026-01-25 17:01 UTC (permalink / raw) Commit 90eae926a fixed ON CONFLICT handling during REINDEX CONCURRENTLY on partitioned tables by treating unparented indexes as potential arbiters. However, there's a remaining race condition: when pg_inherits records are swapped between consecutive calls to get_partition_ancestors(), two different child indexes can appear to have the same parent, causing duplicate entries in the arbiter list and triggering "invalid arbiter index list" errors. Note that this is not a new problem introduced by 90eae926a. The same error could occur before that commit in a slightly different scenario: an index is selected during planning, then index_concurrently_swap commits, and a subsequent call to get_partition_ancestors() uses a new catalog snapshot that sees zero ancestors for that index. Fix by tracking which parent indexes have already been processed. If a subsequent call to get_partition_ancestors() returns a parent we've already seen, treat that index as unparented instead, allowing it to be matched via IsIndexCompatibleAsArbiter() like other concurrent reindex scenarios. Author: Mihail Nikalayeu <[email protected]> Reported-by: Alexander Lakhin <[email protected]> Reviewed-by: Álvaro Herrera <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/executor/execPartition.c | 36 ++++++++++---- .../t/010_index_concurrently_upsert.pl | 47 +++++++++++++++++++ 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 55bdf5c4835..d4625d90ad6 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -28,6 +28,7 @@ #include "partitioning/partprune.h" #include "rewrite/rewriteManip.h" #include "utils/acl.h" +#include "utils/injection_point.h" #include "utils/lsyscache.h" #include "utils/partcache.h" #include "utils/rls.h" @@ -761,7 +762,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, if (rootResultRelInfo->ri_onConflictArbiterIndexes != NIL) { List *unparented_idxs = NIL, - *arbiters_listidxs = NIL; + *arbiters_listidxs = NIL, + *ancestors_seen = NIL; for (int listidx = 0; listidx < leaf_part_rri->ri_NumIndices; listidx++) { @@ -775,13 +777,29 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, * in case REINDEX CONCURRENTLY is working on one of the * arbiters. * - * XXX get_partition_ancestors is slow: it scans pg_inherits - * each time. Consider a syscache or some other way to cache? + * However, if two indexes appear to have the same parent, + * treat the second of these as if it had no parent. This + * sounds counterintuitive, but it can happen if a transaction + * running REINDEX CONCURRENTLY commits right between those + * two indexes are checked by another process in this loop. + * This will have the effect of also treating that second + * index as arbiter. + * + * XXX get_partition_ancestors scans pg_inherits, which is not + * only slow, but also means the catalog snapshot can get + * invalidated each time through the loop (cf. + * GetNonHistoricCatalogSnapshot). Consider a syscache or + * some other way to cache? */ indexoid = RelationGetRelid(leaf_part_rri->ri_IndexRelationDescs[listidx]); ancestors = get_partition_ancestors(indexoid); - if (ancestors != NIL) + INJECTION_POINT("exec-init-partition-after-get-partition-ancestors", NULL); + + if (ancestors != NIL && + !list_member_oid(ancestors_seen, linitial_oid(ancestors))) { + ancestors_seen = lappend_oid(ancestors_seen, linitial_oid(ancestors)); + foreach_oid(parent_idx, rootResultRelInfo->ri_onConflictArbiterIndexes) { if (list_member_oid(ancestors, parent_idx)) @@ -794,6 +812,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, } else unparented_idxs = lappend_int(unparented_idxs, listidx); + list_free(ancestors); } @@ -812,16 +831,16 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, foreach_int(unparented_i, unparented_idxs) { Relation unparented_rel; - IndexInfo *unparenred_ii; + IndexInfo *unparented_ii; unparented_rel = leaf_part_rri->ri_IndexRelationDescs[unparented_i]; - unparenred_ii = leaf_part_rri->ri_IndexRelationInfo[unparented_i]; + unparented_ii = leaf_part_rri->ri_IndexRelationInfo[unparented_i]; Assert(!list_member_oid(arbiterIndexes, unparented_rel->rd_index->indexrelid)); /* Ignore indexes not ready */ - if (!unparenred_ii->ii_ReadyForInserts) + if (!unparented_ii->ii_ReadyForInserts) continue; foreach_int(arbiter_i, arbiters_listidxs) @@ -839,7 +858,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, if (IsIndexCompatibleAsArbiter(arbiter_rel, arbiter_ii, unparented_rel, - unparenred_ii)) + unparented_ii)) { arbiterIndexes = lappend_oid(arbiterIndexes, unparented_rel->rd_index->indexrelid); @@ -851,6 +870,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, } list_free(unparented_idxs); list_free(arbiters_listidxs); + list_free(ancestors_seen); } /* diff --git a/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl b/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl index 26682ebc55a..69178b129c7 100644 --- a/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl +++ b/src/test/modules/test_misc/t/010_index_concurrently_upsert.pl @@ -785,6 +785,53 @@ clean_safe_quit_ok($s1, $s2, $s3); $node->safe_psql('postgres', 'TRUNCATE TABLE test.tblexpr'); +############################################################################ +note('Test: REINDEX on partitioned table (invalid arbiter list)'); + +# causes invalid ERROR: invalid arbiter index list + +$s1 = $node->background_psql('postgres', on_error_stop => 0); +$s2 = $node->background_psql('postgres', on_error_stop => 0); +$s3 = $node->background_psql('postgres', on_error_stop => 0); + +$s1->query_safe( + q[ +SELECT injection_points_set_local(); +SELECT injection_points_attach('exec-init-partition-after-get-partition-ancestors', 'wait'); +]); + +$s2->query_safe( + q[ +SELECT injection_points_set_local(); +SELECT injection_points_attach('reindex-relation-concurrently-before-swap', 'wait'); +]); + +$s2->query_until( + qr/starting_reindex/, q[ +\echo starting_reindex +REINDEX INDEX CONCURRENTLY test.tbl_partition_pkey; +]); + +ok_injection_point($node, 'reindex-relation-concurrently-before-swap'); + +$s1->query_until( + qr/starting_upsert_s1/, q[ +\echo starting_upsert_s1 +INSERT INTO test.tblparted VALUES (13, now()) ON CONFLICT (i) DO UPDATE SET updated_at = now(); +]); + +ok_injection_point($node, + 'exec-init-partition-after-get-partition-ancestors'); + +wakeup_injection_point($node, 'reindex-relation-concurrently-before-swap'); + +wakeup_injection_point($node, + 'exec-init-partition-after-get-partition-ancestors'); + +clean_safe_quit_ok($s1, $s2, $s3); + +$node->safe_psql('postgres', 'TRUNCATE TABLE test.tblparted'); + done_testing(); ############################################################################ -- 2.47.3 --64l6h6ca6ah5fvk6-- ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-01-25 17:01 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2018-02-12 01:35 OCD questions about LWTRANCHE_REPLICATION_ORIGIN Thomas Munro <[email protected]> 2020-02-28 06:52 [PATCH v18] Make End-Of-Recovery error less scary Kyotaro Horiguchi <[email protected]> 2026-01-25 17:01 [PATCH v2] Fix duplicate arbiter detection during REINDEX CONCURRENTLY on partitions Mikhail Nikalayeu <[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