Skip to content

Commit d8e950d

Browse files
anarazelanarazel
committed
Improve and cleanup ProcArrayAdd(), ProcArrayRemove().
941697c changed ProcArrayAdd()/Remove() substantially. As reported by zhanyi, I missed that due to the introduction of the PGPROC->pgxactoff ProcArrayRemove() does not need to search for the position in procArray->pgprocnos anymore - that's pgxactoff. Remove the search loop. The removal of the search loop reduces assertion coverage a bit - but we can easily do better than before by adding assertions to other loops over pgprocnos elements. Also do a bit of cleanup, mostly by reducing line lengths by introducing local helper variables and adding newlines. Author: zhanyi <w@hidva.com> Author: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/tencent_5624AA3B116B3D1C31CA9744@qq.com
1 parent d120e66 commit d8e950d

File tree

1 file changed

+78
-55
lines changed

1 file changed

+78
-55
lines changed

‎src/backend/storage/ipc/procarray.c

Lines changed: 78 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,7 @@ ProcArrayAdd(PGPROC *proc)
446446
{
447447
ProcArrayStruct *arrayP = procArray;
448448
int index;
449+
int movecount;
449450

450451
/* See ProcGlobal comment explaining why both locks are held */
451452
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
@@ -474,33 +475,48 @@ ProcArrayAdd(PGPROC *proc)
474475
*/
475476
for (index = 0; index < arrayP->numProcs; index++)
476477
{
477-
/*
478-
* If we are the first PGPROC or if we have found our right position
479-
* in the array, break
480-
*/
481-
if ((arrayP->pgprocnos[index] == -1) || (arrayP->pgprocnos[index] > proc->pgprocno))
478+
int procno PG_USED_FOR_ASSERTS_ONLY = arrayP->pgprocnos[index];
479+
480+
Assert(procno >= 0 && procno < (arrayP->maxProcs + NUM_AUXILIARY_PROCS));
481+
Assert(allProcs[procno].pgxactoff == index);
482+
483+
/* If we have found our right position in the array, break */
484+
if (arrayP->pgprocnos[index] > proc->pgprocno)
482485
break;
483486
}
484487

485-
memmove(&arrayP->pgprocnos[index + 1], &arrayP->pgprocnos[index],
486-
(arrayP->numProcs - index) * sizeof(*arrayP->pgprocnos));
487-
memmove(&ProcGlobal->xids[index + 1], &ProcGlobal->xids[index],
488-
(arrayP->numProcs - index) * sizeof(*ProcGlobal->xids));
489-
memmove(&ProcGlobal->subxidStates[index + 1], &ProcGlobal->subxidStates[index],
490-
(arrayP->numProcs - index) * sizeof(*ProcGlobal->subxidStates));
491-
memmove(&ProcGlobal->statusFlags[index + 1], &ProcGlobal->statusFlags[index],
492-
(arrayP->numProcs - index) * sizeof(*ProcGlobal->statusFlags));
488+
movecount = arrayP->numProcs - index;
489+
memmove(&arrayP->pgprocnos[index + 1],
490+
&arrayP->pgprocnos[index],
491+
movecount * sizeof(*arrayP->pgprocnos));
492+
memmove(&ProcGlobal->xids[index + 1],
493+
&ProcGlobal->xids[index],
494+
movecount * sizeof(*ProcGlobal->xids));
495+
memmove(&ProcGlobal->subxidStates[index + 1],
496+
&ProcGlobal->subxidStates[index],
497+
movecount * sizeof(*ProcGlobal->subxidStates));
498+
memmove(&ProcGlobal->statusFlags[index + 1],
499+
&ProcGlobal->statusFlags[index],
500+
movecount * sizeof(*ProcGlobal->statusFlags));
493501

494502
arrayP->pgprocnos[index] = proc->pgprocno;
503+
proc->pgxactoff = index;
495504
ProcGlobal->xids[index] = proc->xid;
496505
ProcGlobal->subxidStates[index] = proc->subxidStatus;
497506
ProcGlobal->statusFlags[index] = proc->statusFlags;
498507

499508
arrayP->numProcs++;
500509

510+
/* adjust pgxactoff for all following PGPROCs */
511+
index++;
501512
for (; index < arrayP->numProcs; index++)
502513
{
503-
allProcs[arrayP->pgprocnos[index]].pgxactoff = index;
514+
int procno = arrayP->pgprocnos[index];
515+
516+
Assert(procno >= 0 && procno < (arrayP->maxProcs + NUM_AUXILIARY_PROCS));
517+
Assert(allProcs[procno].pgxactoff == index - 1);
518+
519+
allProcs[procno].pgxactoff = index;
504520
}
505521

506522
/*
@@ -525,7 +541,8 @@ void
525541
ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
526542
{
527543
ProcArrayStruct *arrayP = procArray;
528-
int index;
544+
int myoff;
545+
int movecount;
529546

530547
#ifdef XIDCACHE_DEBUG
531548
/* dump stats at backend shutdown, but not prepared-xact end */
@@ -537,69 +554,75 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
537554
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
538555
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
539556

540-
Assert(ProcGlobal->allProcs[arrayP->pgprocnos[proc->pgxactoff]].pgxactoff == proc->pgxactoff);
557+
myoff = proc->pgxactoff;
558+
559+
Assert(myoff >= 0 && myoff < arrayP->numProcs);
560+
Assert(ProcGlobal->allProcs[arrayP->pgprocnos[myoff]].pgxactoff == myoff);
541561

542562
if (TransactionIdIsValid(latestXid))
543563
{
544-
Assert(TransactionIdIsValid(ProcGlobal->xids[proc->pgxactoff]));
564+
Assert(TransactionIdIsValid(ProcGlobal->xids[myoff]));
545565

546566
/* Advance global latestCompletedXid while holding the lock */
547567
MaintainLatestCompletedXid(latestXid);
548568

549569
/* Same with xactCompletionCount */
550570
ShmemVariableCache->xactCompletionCount++;
551571

552-
ProcGlobal->xids[proc->pgxactoff] = 0;
553-
ProcGlobal->subxidStates[proc->pgxactoff].overflowed = false;
554-
ProcGlobal->subxidStates[proc->pgxactoff].count = 0;
572+
ProcGlobal->xids[myoff] = InvalidTransactionId;
573+
ProcGlobal->subxidStates[myoff].overflowed = false;
574+
ProcGlobal->subxidStates[myoff].count = 0;
555575
}
556576
else
557577
{
558578
/* Shouldn't be trying to remove a live transaction here */
559-
Assert(!TransactionIdIsValid(ProcGlobal->xids[proc->pgxactoff]));
579+
Assert(!TransactionIdIsValid(ProcGlobal->xids[myoff]));
560580
}
561581

562-
Assert(TransactionIdIsValid(ProcGlobal->xids[proc->pgxactoff] == 0));
563-
Assert(TransactionIdIsValid(ProcGlobal->subxidStates[proc->pgxactoff].count == 0));
564-
Assert(TransactionIdIsValid(ProcGlobal->subxidStates[proc->pgxactoff].overflowed == false));
565-
ProcGlobal->statusFlags[proc->pgxactoff] = 0;
582+
Assert(!TransactionIdIsValid(ProcGlobal->xids[myoff]));
583+
Assert(ProcGlobal->subxidStates[myoff].count == 0);
584+
Assert(ProcGlobal->subxidStates[myoff].overflowed == false);
585+
586+
ProcGlobal->statusFlags[myoff] = 0;
587+
588+
/* Keep the PGPROC array sorted. See notes above */
589+
movecount = arrayP->numProcs - myoff - 1;
590+
memmove(&arrayP->pgprocnos[myoff],
591+
&arrayP->pgprocnos[myoff + 1],
592+
movecount * sizeof(*arrayP->pgprocnos));
593+
memmove(&ProcGlobal->xids[myoff],
594+
&ProcGlobal->xids[myoff + 1],
595+
movecount * sizeof(*ProcGlobal->xids));
596+
memmove(&ProcGlobal->subxidStates[myoff],
597+
&ProcGlobal->subxidStates[myoff + 1],
598+
movecount * sizeof(*ProcGlobal->subxidStates));
599+
memmove(&ProcGlobal->statusFlags[myoff],
600+
&ProcGlobal->statusFlags[myoff + 1],
601+
movecount * sizeof(*ProcGlobal->statusFlags));
602+
603+
arrayP->pgprocnos[arrayP->numProcs - 1] = -1; /* for debugging */
604+
arrayP->numProcs--;
566605

567-
for (index = 0; index < arrayP->numProcs; index++)
606+
/*
607+
* Adjust pgxactoff of following procs for removed PGPROC (note that
608+
* numProcs already has been decremented).
609+
*/
610+
for (int index = myoff; index < arrayP->numProcs; index++)
568611
{
569-
if (arrayP->pgprocnos[index] == proc->pgprocno)
570-
{
571-
/* Keep the PGPROC array sorted. See notes above */
572-
memmove(&arrayP->pgprocnos[index], &arrayP->pgprocnos[index + 1],
573-
(arrayP->numProcs - index - 1) * sizeof(*arrayP->pgprocnos));
574-
memmove(&ProcGlobal->xids[index], &ProcGlobal->xids[index + 1],
575-
(arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->xids));
576-
memmove(&ProcGlobal->subxidStates[index], &ProcGlobal->subxidStates[index + 1],
577-
(arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->subxidStates));
578-
memmove(&ProcGlobal->statusFlags[index], &ProcGlobal->statusFlags[index + 1],
579-
(arrayP->numProcs - index - 1) * sizeof(*ProcGlobal->statusFlags));
580-
581-
arrayP->pgprocnos[arrayP->numProcs - 1] = -1; /* for debugging */
582-
arrayP->numProcs--;
583-
584-
/* adjust for removed PGPROC */
585-
for (; index < arrayP->numProcs; index++)
586-
allProcs[arrayP->pgprocnos[index]].pgxactoff--;
612+
int procno = arrayP->pgprocnos[index];
587613

588-
/*
589-
* Release in reversed acquisition order, to reduce frequency of
590-
* having to wait for XidGenLock while holding ProcArrayLock.
591-
*/
592-
LWLockRelease(XidGenLock);
593-
LWLockRelease(ProcArrayLock);
594-
return;
595-
}
614+
Assert(procno >= 0 && procno < (arrayP->maxProcs + NUM_AUXILIARY_PROCS));
615+
Assert(allProcs[procno].pgxactoff - 1 == index);
616+
617+
allProcs[procno].pgxactoff = index;
596618
}
597619

598-
/* Oops */
620+
/*
621+
* Release in reversed acquisition order, to reduce frequency of having to
622+
* wait for XidGenLock while holding ProcArrayLock.
623+
*/
599624
LWLockRelease(XidGenLock);
600625
LWLockRelease(ProcArrayLock);
601-
602-
elog(LOG, "failed to find proc %p in ProcArray", proc);
603626
}
604627

605628

0 commit comments

Comments
 (0)