On Wed, Nov 10, 2021 at 6:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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?
My wrong. It should not be tied.
>
> 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 think we should acquire an exclusive lock only if status flags are
not empty. But to check the status flags we need to acquire a shared
lock. No?
> 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.
Fair enough. I was also concerned that but since
ProcArrayInstallRestoredXmin() is a convenient place to set status
flags I changed the patch accordingly. As you pointed out, doing that
separately for the parallel worker is clearer.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/