On Fri, Oct 22, 2021 at 11:08 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Oct 20, 2021 at 9:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I agree to copy statusFlags in ProcArrayInstallRestoredXmin(). I've
> updated the patch accordingly.
>
1.
@@ -2663,7 +2677,16 @@ ProcArrayInstallRestoredXmin(TransactionId
xmin, PGPROC *proc)
TransactionIdIsNormal(xid) &&
TransactionIdPrecedesOrEquals(xid, xmin))
{
+ /* restore xmin */
MyProc->xmin = TransactionXmin = xmin;
+
+ /* copy statusFlags */
+ if (flags != 0)
+ {
+ MyProc->statusFlags = proc->statusFlags;
+ ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+ }
Is there a reason to tie the logic of copying status flags with the
last two transaction-related conditions?
2.
LWLockAcquire(ProcArrayLock, LW_SHARED);
+ flags = proc->statusFlags;
+
+ /*
+ * If the source xact has any statusFlags, we re-grab ProcArrayLock
+ * on exclusive mode so we can copy it to MyProc->statusFlags.
+ */
+ if (flags != 0)
+ {
+ LWLockRelease(ProcArrayLock);
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ }
This looks a bit odd to me. It would have been better if we know when
to acquire an exclusive lock without first acquiring the shared lock.
I see why it could be a good idea to do this stuff in
ProcArrayInstallRestoredXmin() but seeing the patch it seems better to
do this separately for the parallel worker as is done in your previous
patch version but do it after we call
StartParallelWorkerTransaction(). I am also not very sure if the other
callers of this code path will expect ProcArrayInstallRestoredXmin()
to do this assignment and also the function name appears to be very
specific to what it is currently doing.
--
With Regards,
Amit Kapila.
