public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v8 4/4] Move code to apply one WAL record to a subroutine. 11+ messages / 6 participants [nested] [flat]
* [PATCH v8 4/4] Move code to apply one WAL record to a subroutine. @ 2021-09-16 08:07 Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Heikki Linnakangas @ 2021-09-16 08:07 UTC (permalink / raw) --- src/backend/access/transam/xlogrecovery.c | 267 +++++++++++----------- 1 file changed, 139 insertions(+), 128 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index c21436190fd..72dc611567e 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -367,6 +367,7 @@ static char recoveryStopName[MAXFNAMELEN]; static bool recoveryStopAfter; /* prototypes for local functions */ +static void ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, TimeLineID *replayTLI); static void xlog_block_info(StringInfo buf, XLogReaderState *record); static void readRecoverySignalFile(void); @@ -1398,7 +1399,6 @@ PerformWalRecovery(void) if (record != NULL) { - ErrorContextCallback errcallback; TimestampTz xtime; PGRUsage ru0; @@ -1426,8 +1426,6 @@ PerformWalRecovery(void) */ do { - bool switchedTLI = false; - if (!StandbyMode) ereport_startup_progress("redo in progress, elapsed time: %ld.%02d s, current LSN: %X/%X", LSN_FORMAT_ARGS(xlogreader->ReadRecPtr)); @@ -1497,133 +1495,10 @@ PerformWalRecovery(void) recoveryPausesHere(false); } - /* Setup error traceback support for ereport() */ - errcallback.callback = rm_redo_error_callback; - errcallback.arg = (void *) xlogreader; - errcallback.previous = error_context_stack; - error_context_stack = &errcallback; - - /* - * ShmemVariableCache->nextXid must be beyond record's xid. - */ - AdvanceNextFullTransactionIdPastXid(record->xl_xid); - - /* - * Before replaying this record, check if this record causes the - * current timeline to change. The record is already considered to - * be part of the new timeline, so we update ThisTimeLineID before - * replaying it. That's important so that replayEndTLI, which is - * recorded as the minimum recovery point's TLI if recovery stops - * after this record, is set correctly. - */ - if (record->xl_rmid == RM_XLOG_ID) - { - TimeLineID newReplayTLI = replayTLI; - TimeLineID prevReplayTLI = replayTLI; - uint8 info = record->xl_info & ~XLR_INFO_MASK; - - if (info == XLOG_CHECKPOINT_SHUTDOWN) - { - CheckPoint checkPoint; - - memcpy(&checkPoint, XLogRecGetData(xlogreader), sizeof(CheckPoint)); - newReplayTLI = checkPoint.ThisTimeLineID; - prevReplayTLI = checkPoint.PrevTimeLineID; - } - else if (info == XLOG_END_OF_RECOVERY) - { - xl_end_of_recovery xlrec; - - memcpy(&xlrec, XLogRecGetData(xlogreader), sizeof(xl_end_of_recovery)); - newReplayTLI = xlrec.ThisTimeLineID; - prevReplayTLI = xlrec.PrevTimeLineID; - } - - if (newReplayTLI != replayTLI) - { - /* Check that it's OK to switch to this TLI */ - checkTimeLineSwitch(xlogreader->EndRecPtr, newReplayTLI, - prevReplayTLI, replayTLI); - - /* Following WAL records should be run with new TLI */ - replayTLI = newReplayTLI; - switchedTLI = true; - } - } - - /* - * Update shared replayEndRecPtr before replaying this record, so - * that XLogFlush will update minRecoveryPoint correctly. - */ - SpinLockAcquire(&XLogRecoveryCtl->info_lck); - XLogRecoveryCtl->replayEndRecPtr = xlogreader->EndRecPtr; - XLogRecoveryCtl->replayEndTLI = replayTLI; - SpinLockRelease(&XLogRecoveryCtl->info_lck); - - /* - * If we are attempting to enter Hot Standby mode, process XIDs we - * see - */ - if (standbyState >= STANDBY_INITIALIZED && - TransactionIdIsValid(record->xl_xid)) - RecordKnownAssignedTransactionIds(record->xl_xid); - - /* Now apply the WAL record itself */ - RmgrTable[record->xl_rmid].rm_redo(xlogreader); - - /* - * After redo, check whether the backup pages associated with the - * WAL record are consistent with the existing pages. This check - * is done only if consistency check is enabled for this record. - */ - if ((record->xl_info & XLR_CHECK_CONSISTENCY) != 0) - checkXLogConsistency(xlogreader); - - /* Pop the error context stack */ - error_context_stack = errcallback.previous; - - /* - * Update lastReplayedEndRecPtr after this record has been - * successfully replayed. - */ - SpinLockAcquire(&XLogRecoveryCtl->info_lck); - XLogRecoveryCtl->lastReplayedEndRecPtr = xlogreader->EndRecPtr; - XLogRecoveryCtl->lastReplayedTLI = replayTLI; - SpinLockRelease(&XLogRecoveryCtl->info_lck); - - /* Also remember its starting position. */ - LastReplayedReadRecPtr = xlogreader->ReadRecPtr; - /* - * If rm_redo called XLogRequestWalReceiverReply, then we wake up - * the receiver so that it notices the updated - * lastReplayedEndRecPtr and sends a reply to the primary. + * Apply the record */ - if (doRequestWalReceiverReply) - { - doRequestWalReceiverReply = false; - WalRcvForceReply(); - } - - /* Allow read-only connections if we're consistent now */ - CheckRecoveryConsistency(); - - /* Is this a timeline switch? */ - if (switchedTLI) - { - /* - * Before we continue on the new timeline, clean up any - * (possibly bogus) future WAL segments on the old timeline. - */ - RemoveNonParentXlogFiles(xlogreader->EndRecPtr, replayTLI); - - /* - * Wake up any walsenders to notice that we are on a new - * timeline. - */ - if (AllowCascadeReplication()) - WalSndWakeup(); - } + ApplyWalRecord(xlogreader, record, &replayTLI); /* Exit loop if we reached inclusive recovery target */ if (recoveryStopsAfter(xlogreader)) @@ -1711,6 +1586,142 @@ PerformWalRecovery(void) (errmsg("recovery ended before configured recovery target was reached"))); } +/* + * Subroutine of PerformWalRecovery, to apply one WAL record. + */ +static void +ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, TimeLineID *replayTLI) +{ + ErrorContextCallback errcallback; + bool switchedTLI = false; + + /* Setup error traceback support for ereport() */ + errcallback.callback = rm_redo_error_callback; + errcallback.arg = (void *) xlogreader; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + + /* + * ShmemVariableCache->nextXid must be beyond record's xid. + */ + AdvanceNextFullTransactionIdPastXid(record->xl_xid); + + /* + * Before replaying this record, check if this record causes the current + * timeline to change. The record is already considered to be part of the + * new timeline, so we update replayTLI before replaying it. That's + * important so that replayEndTLI, which is recorded as the minimum + * recovery point's TLI if recovery stops after this record, is set + * correctly. + */ + if (record->xl_rmid == RM_XLOG_ID) + { + TimeLineID newReplayTLI = *replayTLI; + TimeLineID prevReplayTLI = *replayTLI; + uint8 info = record->xl_info & ~XLR_INFO_MASK; + + if (info == XLOG_CHECKPOINT_SHUTDOWN) + { + CheckPoint checkPoint; + + memcpy(&checkPoint, XLogRecGetData(xlogreader), sizeof(CheckPoint)); + newReplayTLI = checkPoint.ThisTimeLineID; + prevReplayTLI = checkPoint.PrevTimeLineID; + } + else if (info == XLOG_END_OF_RECOVERY) + { + xl_end_of_recovery xlrec; + + memcpy(&xlrec, XLogRecGetData(xlogreader), sizeof(xl_end_of_recovery)); + newReplayTLI = xlrec.ThisTimeLineID; + prevReplayTLI = xlrec.PrevTimeLineID; + } + + if (newReplayTLI != *replayTLI) + { + /* Check that it's OK to switch to this TLI */ + checkTimeLineSwitch(xlogreader->EndRecPtr, + newReplayTLI, prevReplayTLI, *replayTLI); + + /* Following WAL records should be run with new TLI */ + *replayTLI = newReplayTLI; + switchedTLI = true; + } + } + + /* + * Update shared replayEndRecPtr before replaying this record, so that + * XLogFlush will update minRecoveryPoint correctly. + */ + SpinLockAcquire(&XLogRecoveryCtl->info_lck); + XLogRecoveryCtl->replayEndRecPtr = xlogreader->EndRecPtr; + XLogRecoveryCtl->replayEndTLI = *replayTLI; + SpinLockRelease(&XLogRecoveryCtl->info_lck); + + /* + * If we are attempting to enter Hot Standby mode, process XIDs we see + */ + if (standbyState >= STANDBY_INITIALIZED && + TransactionIdIsValid(record->xl_xid)) + RecordKnownAssignedTransactionIds(record->xl_xid); + + /* Now apply the WAL record itself */ + RmgrTable[record->xl_rmid].rm_redo(xlogreader); + + /* + * After redo, check whether the backup pages associated with the WAL + * record are consistent with the existing pages. This check is done only + * if consistency check is enabled for this record. + */ + if ((record->xl_info & XLR_CHECK_CONSISTENCY) != 0) + checkXLogConsistency(xlogreader); + + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + + /* + * Update lastReplayedEndRecPtr after this record has been successfully + * replayed. + */ + SpinLockAcquire(&XLogRecoveryCtl->info_lck); + XLogRecoveryCtl->lastReplayedEndRecPtr = xlogreader->EndRecPtr; + XLogRecoveryCtl->lastReplayedTLI = *replayTLI; + SpinLockRelease(&XLogRecoveryCtl->info_lck); + + /* Also remember its starting position. */ + LastReplayedReadRecPtr = xlogreader->ReadRecPtr; + + /* + * If rm_redo called XLogRequestWalReceiverReply, then we wake up the + * receiver so that it notices the updated lastReplayedEndRecPtr and sends + * a reply to the primary. + */ + if (doRequestWalReceiverReply) + { + doRequestWalReceiverReply = false; + WalRcvForceReply(); + } + + /* Allow read-only connections if we're consistent now */ + CheckRecoveryConsistency(); + + /* Is this a timeline switch? */ + if (switchedTLI) + { + /* + * Before we continue on the new timeline, clean up any (possibly + * bogus) future WAL segments on the old timeline. + */ + RemoveNonParentXlogFiles(xlogreader->EndRecPtr, *replayTLI); + + /* + * Wake up any walsenders to notice that we are on a new timeline. + */ + if (AllowCascadeReplication()) + WalSndWakeup(); + } +} + /* * Error context callback for errors occurring during rm_redo(). */ -- 2.30.2 --------------2135AFA2DD79B96A7476E2B5-- ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-05 02:37 Richard Guo <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Richard Guo @ 2023-06-05 02:37 UTC (permalink / raw) To: Ranier Vilela <[email protected]>; +Cc: pgsql-hackers On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela <[email protected]> wrote: > Hi, > > Per Coverity. > > At function ExtendBufferedRelShared, has a always true test. > eb.rel was dereferenced one line above, so in > if (eb.rel) is always true. > > I think it's worth removing the test, because Coverity raises dozens of > alerts thinking eb.rel might be NULL. > Besides, one less test is one less branch. > This also happens in ExtendBufferedRelTo, and the comment there explains that the eb.rel 'could have been closed while waiting for lock'. So for the same consideration, the test in ExtendBufferedRelShared might be still needed? But I'm not familiar with the arounding codes, so need someone else to confirm that. Thanks Richard ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-05 11:06 Ranier Vilela <[email protected]> parent: Richard Guo <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Ranier Vilela @ 2023-06-05 11:06 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <[email protected]> escreveu: > > On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela <[email protected]> wrote: > >> Hi, >> >> Per Coverity. >> >> At function ExtendBufferedRelShared, has a always true test. >> eb.rel was dereferenced one line above, so in >> if (eb.rel) is always true. >> >> I think it's worth removing the test, because Coverity raises dozens of >> alerts thinking eb.rel might be NULL. >> Besides, one less test is one less branch. >> > > This also happens in ExtendBufferedRelTo, and the comment there explains > that the eb.rel 'could have been closed while waiting for lock'. > Well, RelationGetSmgr also dereferences eb.rel. If eb.rel could be closed while waiting for lock, anyone who references eb.rel below takes a risk? static inline SMgrRelation RelationGetSmgr(Relation rel) { if (unlikely(rel->rd_smgr == NULL)) smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend)); return rel->rd_smgr; } regards, Ranier Vilela ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-05 11:24 Ranier Vilela <[email protected]> parent: Ranier Vilela <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Ranier Vilela @ 2023-06-05 11:24 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers Em seg., 5 de jun. de 2023 às 08:06, Ranier Vilela <[email protected]> escreveu: > Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <[email protected]> > escreveu: > >> >> On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela <[email protected]> wrote: >> >>> Hi, >>> >>> Per Coverity. >>> >>> At function ExtendBufferedRelShared, has a always true test. >>> eb.rel was dereferenced one line above, so in >>> if (eb.rel) is always true. >>> >>> I think it's worth removing the test, because Coverity raises dozens of >>> alerts thinking eb.rel might be NULL. >>> Besides, one less test is one less branch. >>> >> >> This also happens in ExtendBufferedRelTo, and the comment there explains >> that the eb.rel 'could have been closed while waiting for lock'. >> > Well, RelationGetSmgr also dereferences eb.rel. > If eb.rel could be closed while waiting for lock, > anyone who references eb.rel below takes a risk? > > static inline SMgrRelation > RelationGetSmgr(Relation rel) > { > if (unlikely(rel->rd_smgr == NULL)) > smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend)); > return rel->rd_smgr; > } > Sorry Richard, nevermind. My fault, I withdraw this patch. regards, Ranier Vilela ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-13 05:51 Gurjeet Singh <[email protected]> parent: Ranier Vilela <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Gurjeet Singh @ 2023-06-13 05:51 UTC (permalink / raw) To: Ranier Vilela <[email protected]>; +Cc: Richard Guo <[email protected]>; pgsql-hackers On Mon, Jun 5, 2023 at 4:24 AM Ranier Vilela <[email protected]> wrote: > Em seg., 5 de jun. de 2023 às 08:06, Ranier Vilela <[email protected]> escreveu: >> Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <[email protected]> escreveu: >>> On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela <[email protected]> wrote: >>>> Hi, >>>> >>>> Per Coverity. >>>> >>>> At function ExtendBufferedRelShared, has a always true test. >>>> eb.rel was dereferenced one line above, so in >>>> if (eb.rel) is always true. >>>> >>>> I think it's worth removing the test, because Coverity raises dozens of alerts thinking eb.rel might be NULL. >>>> Besides, one less test is one less branch. >>> >>> >>> This also happens in ExtendBufferedRelTo, and the comment there explains >>> that the eb.rel 'could have been closed while waiting for lock'. >> >> Well, RelationGetSmgr also dereferences eb.rel. >> If eb.rel could be closed while waiting for lock, >> anyone who references eb.rel below takes a risk? >> >> static inline SMgrRelation >> RelationGetSmgr(Relation rel) >> { >> if (unlikely(rel->rd_smgr == NULL)) >> smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend)); >> return rel->rd_smgr; >> } > > Sorry Richard, nevermind. > > My fault, I withdraw this patch. I'm not quite convinced of the reasoning provided; either the reason is not good enough, or my C is rusty. In either case, I'd like a resolution. The code in question: > LockRelationForExtension(eb.rel, ExclusiveLock); > > /* could have been closed while waiting for lock */ > if (eb.rel) > eb.smgr = RelationGetSmgr(eb.rel); eb.rel is being passed by-value at line 1, so even if the relation is closed, the value of the eb.rel cannot change between line 1 and line 3. So a code verification tool complaining that the 'if' condition will always be true is quite right, IMO. To verify my assumptions, I removed those checks and ran `make {check,check-world,installcheck}`, and all those tests passed. The only way, that I can think of, the value of eb.rel can change between lines 1 and 3 is if 'eb' is a shared-memory data structure, and some other process changed the 'rel' member in shared-memory. And I don't think 'eb' is in shared memory in this case. To me, it looks like these checks are a result of code being copy-pasted from somewhere else, where this check might have been necessary. The checks are sure not necessary at these spots. Please see attached v2 of the patch; it includes both occurrences of the spurious checks identified in this thread. Best regards, Gurjeet http://Gurje.et Attachments: [application/octet-stream] v2-0001-Remove-always-true-checks.patch (1.2K, ../../CABwTF4W+WMr=3bjLP5-69+oG81BEZOH+SAqPX8GDnZry5==thA@mail.gmail.com/2-v2-0001-Remove-always-true-checks.patch) download | inline diff: From 4f0cd59bfed63e5fd2d9472f1b8fc5fe308e9431 Mon Sep 17 00:00:00 2001 From: Gurjeet Singh <[email protected]> Date: Mon, 12 Jun 2023 22:39:33 -0700 Subject: [PATCH v2] Remove always true_checks The lines above the check had just referenced the struct member, so the check is unnecessary, as it will always yeild true. --- src/backend/storage/buffer/bufmgr.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 3c59bbd04e..dd418f3814 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -907,9 +907,7 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb, { LockRelationForExtension(eb.rel, ExclusiveLock); - /* could have been closed while waiting for lock */ - if (eb.rel) - eb.smgr = RelationGetSmgr(eb.rel); + eb.smgr = RelationGetSmgr(eb.rel); /* recheck, fork might have been created concurrently */ if (!smgrexists(eb.smgr, fork)) @@ -1875,8 +1873,7 @@ ExtendBufferedRelShared(ExtendBufferedWhat eb, if (!(flags & EB_SKIP_EXTENSION_LOCK)) { LockRelationForExtension(eb.rel, ExclusiveLock); - if (eb.rel) - eb.smgr = RelationGetSmgr(eb.rel); + eb.smgr = RelationGetSmgr(eb.rel); } /* -- 2.35.1 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-13 06:11 Michael Paquier <[email protected]> parent: Gurjeet Singh <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Michael Paquier @ 2023-06-13 06:11 UTC (permalink / raw) To: Gurjeet Singh <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Richard Guo <[email protected]>; pgsql-hackers On Mon, Jun 12, 2023 at 10:51:24PM -0700, Gurjeet Singh wrote: > To me, it looks like these checks are a result of code being > copy-pasted from somewhere else, where this check might have been > necessary. The checks are sure not necessary at these spots. I am not completely sure based on my read of the code, but isn't this check needed to avoid some kind of race condition with a concurrent backend may have worked on the relation when attempting to get the lock? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-13 07:39 Kyotaro Horiguchi <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Kyotaro Horiguchi @ 2023-06-13 07:39 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers At Tue, 13 Jun 2023 15:11:26 +0900, Michael Paquier <[email protected]> wrote in > On Mon, Jun 12, 2023 at 10:51:24PM -0700, Gurjeet Singh wrote: > > To me, it looks like these checks are a result of code being > > copy-pasted from somewhere else, where this check might have been > > necessary. The checks are sure not necessary at these spots. > > I am not completely sure based on my read of the code, but isn't this > check needed to avoid some kind of race condition with a concurrent > backend may have worked on the relation when attempting to get the > lock? Gurjeet has mentioned that eb.rel cannot be modified by another process since the value or memory is in the local stack, and I believe he's correct. If the pointed Relation had been blown out, eb.rel would be left dangling, not nullified. However, I don't believe this situation happens (or it shouldn't happen) as the entire relation should already be locked. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-13 11:21 Ranier Vilela <[email protected]> parent: Gurjeet Singh <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Ranier Vilela @ 2023-06-13 11:21 UTC (permalink / raw) To: Gurjeet Singh <[email protected]>; +Cc: Richard Guo <[email protected]>; pgsql-hackers Em ter., 13 de jun. de 2023 às 02:51, Gurjeet Singh <[email protected]> escreveu: > Please see attached v2 of the patch; it includes both occurrences of > the spurious checks identified in this thread. > +1 LGTM. regards, Ranier Vilela ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-14 01:01 Kyotaro Horiguchi <[email protected]> parent: Ranier Vilela <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Kyotaro Horiguchi @ 2023-06-14 01:01 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers At Tue, 13 Jun 2023 08:21:20 -0300, Ranier Vilela <[email protected]> wrote in > Em ter., 13 de jun. de 2023 às 02:51, Gurjeet Singh <[email protected]> > escreveu: > > > Please see attached v2 of the patch; it includes both occurrences of > > the spurious checks identified in this thread. > > > +1 > LGTM. > LockRelationForExtension(eb.rel, ExclusiveLock); > > - /* could have been closed while waiting for lock */ > - if (eb.rel) > - eb.smgr = RelationGetSmgr(eb.rel); > + eb.smgr = RelationGetSmgr(eb.rel); (It seems to me) The removed comment does refer to smgr, so we could consider keeping it. There are places instances where the function calls are accompanied by similar comments and others where they aren't. However, personally, I inclined towards its removal. That's because our policy is to call RelationGetSmgr() each time before using smgr, and this is well documented in the function's comment. If we decide to remove it, the preceding blank line seems to be a separator from the previous function call. So, we might want to consider removing that blank line, too. Otherwise it LGTM. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-14 01:05 Kyotaro Horiguchi <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Kyotaro Horiguchi @ 2023-06-14 01:05 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers At Wed, 14 Jun 2023 10:01:59 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > If we decide to remove it, the preceding blank line seems to be a > separator from the previous function call. So, we might want to Mmm. that is a bit short. Anyway I meant that the blank will become useless after removing the comment. > consider removing that blank line, too. > > Otherwise it LGTM. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) @ 2023-06-14 09:50 Richard Guo <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Richard Guo @ 2023-06-14 09:50 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers On Tue, Jun 13, 2023 at 3:39 PM Kyotaro Horiguchi <[email protected]> wrote: > Gurjeet has mentioned that eb.rel cannot be modified by another > process since the value or memory is in the local stack, and I believe > he's correct. > > If the pointed Relation had been blown out, eb.rel would be left > dangling, not nullified. However, I don't believe this situation > happens (or it shouldn't happen) as the entire relation should already > be locked. Yeah, Gurjeet is right. I had a thinko here. eb.rel should not be NULL pointer in any case. And as we've acquired the lock for it, it should not have been closed. So I think we can remove the check for eb.rel in the two places. Thanks Richard ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-06-14 09:50 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-09-16 08:07 [PATCH v8 4/4] Move code to apply one WAL record to a subroutine. Heikki Linnakangas <[email protected]> 2023-06-05 02:37 Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Richard Guo <[email protected]> 2023-06-05 11:06 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Ranier Vilela <[email protected]> 2023-06-05 11:24 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Ranier Vilela <[email protected]> 2023-06-13 05:51 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Gurjeet Singh <[email protected]> 2023-06-13 06:11 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Michael Paquier <[email protected]> 2023-06-13 07:39 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Kyotaro Horiguchi <[email protected]> 2023-06-14 09:50 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Richard Guo <[email protected]> 2023-06-13 11:21 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Ranier Vilela <[email protected]> 2023-06-14 01:01 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Kyotaro Horiguchi <[email protected]> 2023-06-14 01:05 ` Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c) Kyotaro Horiguchi <[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