Thread: Parallel vacuum workers prevent the oldest xmin from advancing
Hi all, A customer reported that during parallel index vacuum, the oldest xmin doesn't advance. Normally, the calculation of oldest xmin (ComputeXidHorizons()) ignores xmin/xid of processes having PROC_IN_VACUUM flag in MyProc->statusFlags. But since parallel vacuum workers don’t set their statusFlags, the xmin of the parallel vacuum worker is considered to calculate the oldest xmin. This issue happens from PG13 where the parallel vacuum was introduced. I think it's a bug. Moreover, the same problem happens also in CREATE/REINDEX CONCURRENTLY case in PG14 or later for the same reason (due to lack of PROC_IN_SAFE_IC flag). To fix it, I thought that we change the create index code and the vacuum code so that the individual parallel worker sets its status flags according to the leader’s one. But ISTM it’d be better to copy the leader’s status flags to workers in ParallelWorkerMain(). I've attached a patch for HEAD. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
On 10/6/21, 12:13 AM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote: > A customer reported that during parallel index vacuum, the oldest xmin > doesn't advance. Normally, the calculation of oldest xmin > (ComputeXidHorizons()) ignores xmin/xid of processes having > PROC_IN_VACUUM flag in MyProc->statusFlags. But since parallel vacuum > workers don’t set their statusFlags, the xmin of the parallel vacuum > worker is considered to calculate the oldest xmin. This issue happens > from PG13 where the parallel vacuum was introduced. I think it's a > bug. +1 > To fix it, I thought that we change the create index code and the > vacuum code so that the individual parallel worker sets its status > flags according to the leader’s one. But ISTM it’d be better to copy > the leader’s status flags to workers in ParallelWorkerMain(). I've > attached a patch for HEAD. The patch seems reasonable to me. Nathan
On Wed, Oct 6, 2021 at 12:41 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > Hi all, > > A customer reported that during parallel index vacuum, the oldest xmin > doesn't advance. Normally, the calculation of oldest xmin > (ComputeXidHorizons()) ignores xmin/xid of processes having > PROC_IN_VACUUM flag in MyProc->statusFlags. But since parallel vacuum > workers don’t set their statusFlags, the xmin of the parallel vacuum > worker is considered to calculate the oldest xmin. This issue happens > from PG13 where the parallel vacuum was introduced. I think it's a > bug. > I agree. Your patch seems to be in the right direction but I haven't tested it yet. Feel free to register in the next CF to avoid forgetting it. -- With Regards, Amit Kapila.
On 2021-Oct-06, Masahiko Sawada wrote: > Hi all, > > A customer reported that during parallel index vacuum, the oldest xmin > doesn't advance. Normally, the calculation of oldest xmin > (ComputeXidHorizons()) ignores xmin/xid of processes having > PROC_IN_VACUUM flag in MyProc->statusFlags. But since parallel vacuum > workers don’t set their statusFlags, the xmin of the parallel vacuum > worker is considered to calculate the oldest xmin. This issue happens > from PG13 where the parallel vacuum was introduced. I think it's a > bug. Augh, yeah, I agree this is a pretty serious problem. > But ISTM it’d be better to copy the leader’s status flags to workers > in ParallelWorkerMain(). I've attached a patch for HEAD. Hmm, this affects not only PROC_IN_VACUUM and PROC_IN_SAFE_CIC (the bug you're fixing), but also: * PROC_IS_AUTOVACUUM. That seems reasonable to me -- should a parallel worker for autovacuum be considered autovacuum too? AFAICS it's only used by the deadlock detector, so it should be okay. However, in the normal path, that flag is set much earlier. * PROC_VACUUM_FOR_WRAPAROUND. Should be innocuous I think, since the "parent" process already has this flag and thus shouldn't be cancelled. * PROC_IN_LOGICAL_DECODING. Surely not set for parallel vacuum workers, so not a problem. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Doing what he did amounts to sticking his fingers under the hood of the implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)
On Fri, Oct 8, 2021 at 8:13 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2021-Oct-06, Masahiko Sawada wrote: > > A customer reported that during parallel index vacuum, the oldest xmin > > doesn't advance. Normally, the calculation of oldest xmin > > (ComputeXidHorizons()) ignores xmin/xid of processes having > > PROC_IN_VACUUM flag in MyProc->statusFlags. But since parallel vacuum > > workers don’t set their statusFlags, the xmin of the parallel vacuum > > worker is considered to calculate the oldest xmin. This issue happens > > from PG13 where the parallel vacuum was introduced. I think it's a > > bug. > > Augh, yeah, I agree this is a pretty serious problem. So is this comparable problem, which happens to be much older: https://postgr.es/m/CAH2-WzkjrK556enVtFLmyXEdw91xGuwiyZVep2kp5yQT_-3JDg@mail.gmail.com In both cases we see bugs (or implementation deficiencies) that accidentally block ComputeXidHorizons() for hours, when that isn't truly necessary. Practically all users are not sure of whether or not VACUUM behaves like a long running transaction already, in general, so we shouldn't be surprised that it takes so long for us to hear about issues like this. I think that we should try to find a way of making this whole class of problems easier to identify in production. There needs to be greater visibility into what process holds back VACUUM, and how long that lasts -- something easy to use, and *obvious*. That would be a very useful feature in general. It would also make catching these issues early far more likely. It's just *not okay* that you have to follow long and complicated instructions [1] to get just some of this information. How can something this important just be an afterthought? [1] https://www.cybertec-postgresql.com/en/reasons-why-vacuum-wont-remove-dead-rows/ -- Peter Geoghegan
On Sat, Oct 9, 2021 at 12:13 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Oct-06, Masahiko Sawada wrote: > > > Hi all, > > > > A customer reported that during parallel index vacuum, the oldest xmin > > doesn't advance. Normally, the calculation of oldest xmin > > (ComputeXidHorizons()) ignores xmin/xid of processes having > > PROC_IN_VACUUM flag in MyProc->statusFlags. But since parallel vacuum > > workers don’t set their statusFlags, the xmin of the parallel vacuum > > worker is considered to calculate the oldest xmin. This issue happens > > from PG13 where the parallel vacuum was introduced. I think it's a > > bug. > > Augh, yeah, I agree this is a pretty serious problem. > > > But ISTM it’d be better to copy the leader’s status flags to workers > > in ParallelWorkerMain(). I've attached a patch for HEAD. > > Hmm, this affects not only PROC_IN_VACUUM and PROC_IN_SAFE_CIC (the bug > you're fixing), but also: > > * PROC_IS_AUTOVACUUM. That seems reasonable to me -- should a parallel > worker for autovacuum be considered autovacuum too? AFAICS it's only > used by the deadlock detector, so it should be okay. However, in the > normal path, that flag is set much earlier. > > * PROC_VACUUM_FOR_WRAPAROUND. Should be innocuous I think, since the > "parent" process already has this flag and thus shouldn't be cancelled. Currently, we don't support parallel vacuum for autovacuum. So parallel workers for vacuum don't have these two flags. > * PROC_IN_LOGICAL_DECODING. Surely not set for parallel vacuum workers, > so not a problem. Agreed. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
On Mon, Oct 11, 2021 at 09:23:32AM +0900, Masahiko Sawada wrote: > On Sat, Oct 9, 2021 at 12:13 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> * PROC_VACUUM_FOR_WRAPAROUND. Should be innocuous I think, since the >> "parent" process already has this flag and thus shouldn't be cancelled. > > Currently, we don't support parallel vacuum for autovacuum. So > parallel workers for vacuum don't have these two flags. That's something that should IMO be marked in the code as a comment as something to worry about once/if someone begins playing with parallel autovacuum. If the change involving autovacuum is simple, I see no reason to not add this part now, though. -- Michael
Attachment
On Wed, Oct 6, 2021 at 6:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > To fix it, I thought that we change the create index code and the > vacuum code so that the individual parallel worker sets its status > flags according to the leader’s one. But ISTM it’d be better to copy > the leader’s status flags to workers in ParallelWorkerMain(). I've > attached a patch for HEAD. > +1 The fix looks reasonable to me too. Is it possible for the patch to add test cases for the two identified problem scenarios? (PROC_IN_VACUUM, PROC_IN_SAFE_IC) Regards, Greg Nancarrow Fujitsu Australia
On Mon, Oct 11, 2021 at 3:01 PM Greg Nancarrow <gregn4422@gmail.com> wrote: > > On Wed, Oct 6, 2021 at 6:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > To fix it, I thought that we change the create index code and the > > vacuum code so that the individual parallel worker sets its status > > flags according to the leader’s one. But ISTM it’d be better to copy > > the leader’s status flags to workers in ParallelWorkerMain(). I've > > attached a patch for HEAD. > > > > +1 The fix looks reasonable to me too. > Is it possible for the patch to add test cases for the two identified > problem scenarios? (PROC_IN_VACUUM, PROC_IN_SAFE_IC) Not sure we can add stable tests for this. There is no way in the test infra to control parallel workers to suspend and resume etc. and the oldest xmin can vary depending on the situation. Probably we can add an assertion to ensure a parallel worker for vacuum or create index has PROC_IN_VACUUM or PROC_IN_SAFE_IC, respectively. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
On Mon, Oct 11, 2021 at 9:51 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Oct 11, 2021 at 09:23:32AM +0900, Masahiko Sawada wrote: > > On Sat, Oct 9, 2021 at 12:13 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > >> * PROC_VACUUM_FOR_WRAPAROUND. Should be innocuous I think, since the > >> "parent" process already has this flag and thus shouldn't be cancelled. > > > > Currently, we don't support parallel vacuum for autovacuum. So > > parallel workers for vacuum don't have these two flags. > > That's something that should IMO be marked in the code as a comment as > something to worry about once/if someone begins playing with parallel > autovacuum. If the change involving autovacuum is simple, I see no > reason to not add this part now, though. Agreed. I added the comment. Also, I added an assertion to ensure that a parallel worker for vacuum has PROC_IN_VACUUM flag (and doesn't have other flags). But we cannot do that for parallel workers for building btree index as they don’t know whether or not the CONCURRENTLY option is specified. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
Hmm, I think this should happen before the transaction snapshot is established in the worker; perhaps immediately after calling StartParallelWorkerTransaction(), or anyway not after SetTransactionSnapshot. In fact, since SetTransactionSnapshot receives a 'sourceproc' argument, why not do it exactly there? ISTM that ProcArrayInstallRestoredXmin() is where this should happen. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On 2021-Oct-19, Alvaro Herrera wrote: > Hmm, I think this should happen before the transaction snapshot is > established in the worker; perhaps immediately after calling > StartParallelWorkerTransaction(), or anyway not after > SetTransactionSnapshot. In fact, since SetTransactionSnapshot receives > a 'sourceproc' argument, why not do it exactly there? ISTM that > ProcArrayInstallRestoredXmin() is where this should happen. ... and there is a question about the lock strength used for ProcArrayLock. The current routine uses LW_SHARED, but there's no clarity that we can modify proc->statusFlags and ProcGlobal->statusFlags without LW_EXCLUSIVE. Maybe we can change ProcArrayInstallRestoredXmin so that if it sees that proc->statusFlags is not zero, then it grabs LW_EXCLUSIVE (and copies), otherwise it keeps using LW_SHARED as it does now (and does not copy.) (This also suggests that using LW_EXCLUSIVE inconditionally for all cases as your patch does is not great. OTOH it's just once at every bgworker start, so it's not *that* frequent.) Initially, I was a bit nervous about copying flags willy-nilly. Do we need to be more careful? I mean, have a way for the code to specify flags to copy, maybe something like MyProc->statusFlags |= proc->statusFlags & copyableFlags; ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; with this coding, 1. we do not unset flags that the bgworker already has for whatever reason 2. we do not copy flags that may be unrelated to the effect we desire. The problem, and it's something I don't have an answer for, is how to specify copyableFlags. This code is the generic ParallelWorkerMain() and there's little-to-no chance to pass stuff from the process that requested the bgworker. So maybe Sawada-san's original coding of just copying everything is okay. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
On Wed, Oct 20, 2021 at 3:07 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Oct-19, Alvaro Herrera wrote: > Thank you for the comment. > > Hmm, I think this should happen before the transaction snapshot is > > established in the worker; perhaps immediately after calling > > StartParallelWorkerTransaction(), or anyway not after > > SetTransactionSnapshot. In fact, since SetTransactionSnapshot receives > > a 'sourceproc' argument, why not do it exactly there? ISTM that > > ProcArrayInstallRestoredXmin() is where this should happen. > > ... and there is a question about the lock strength used for > ProcArrayLock. The current routine uses LW_SHARED, but there's no > clarity that we can modify proc->statusFlags and ProcGlobal->statusFlags > without LW_EXCLUSIVE. > > Maybe we can change ProcArrayInstallRestoredXmin so that if it sees that > proc->statusFlags is not zero, then it grabs LW_EXCLUSIVE (and copies), > otherwise it keeps using LW_SHARED as it does now (and does not copy.) Initially, I've considered copying statusFlags in ProcArrayInstallRestoredXmin() but I hesitated to do that because statusFlags is not relevant with xmin and snapshot stuff. But I agree that copying statusFlags should happen before restoring the snapshot. If we copy statusFlags in ProcArrayInstallRestoredXmin() there is still little window that the restored snapshot holds back the oldest xmin? If so it would be better to call ProcArrayCopyStatusFlags() right after StartParallelWorker(). > (This also suggests that using LW_EXCLUSIVE inconditionally for all > cases as your patch does is not great. OTOH it's just once at every > bgworker start, so it's not *that* frequent.) Agreed. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
On Wed, Oct 20, 2021 at 9:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 3:07 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2021-Oct-19, Alvaro Herrera wrote: > > > > Thank you for the comment. > > > > Hmm, I think this should happen before the transaction snapshot is > > > established in the worker; perhaps immediately after calling > > > StartParallelWorkerTransaction(), or anyway not after > > > SetTransactionSnapshot. In fact, since SetTransactionSnapshot receives > > > a 'sourceproc' argument, why not do it exactly there? ISTM that > > > ProcArrayInstallRestoredXmin() is where this should happen. > > > > ... and there is a question about the lock strength used for > > ProcArrayLock. The current routine uses LW_SHARED, but there's no > > clarity that we can modify proc->statusFlags and ProcGlobal->statusFlags > > without LW_EXCLUSIVE. > > > > Maybe we can change ProcArrayInstallRestoredXmin so that if it sees that > > proc->statusFlags is not zero, then it grabs LW_EXCLUSIVE (and copies), > > otherwise it keeps using LW_SHARED as it does now (and does not copy.) > > Initially, I've considered copying statusFlags in > ProcArrayInstallRestoredXmin() but I hesitated to do that because > statusFlags is not relevant with xmin and snapshot stuff. But I agree > that copying statusFlags should happen before restoring the snapshot. > > If we copy statusFlags in ProcArrayInstallRestoredXmin() there is > still little window that the restored snapshot holds back the oldest > xmin? That's wrong, I'd misunderstood. I agree to copy statusFlags in ProcArrayInstallRestoredXmin(). I've updated the patch accordingly. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
On Fri, 22 Oct 2021 at 07:38, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 9:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 3:07 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > > > On 2021-Oct-19, Alvaro Herrera wrote: > > > > > > > Thank you for the comment. > > > > > > Hmm, I think this should happen before the transaction snapshot is > > > > established in the worker; perhaps immediately after calling > > > > StartParallelWorkerTransaction(), or anyway not after > > > > SetTransactionSnapshot. In fact, since SetTransactionSnapshot receives > > > > a 'sourceproc' argument, why not do it exactly there? ISTM that > > > > ProcArrayInstallRestoredXmin() is where this should happen. > > > > > > ... and there is a question about the lock strength used for > > > ProcArrayLock. The current routine uses LW_SHARED, but there's no > > > clarity that we can modify proc->statusFlags and ProcGlobal->statusFlags > > > without LW_EXCLUSIVE. > > > > > > Maybe we can change ProcArrayInstallRestoredXmin so that if it sees that > > > proc->statusFlags is not zero, then it grabs LW_EXCLUSIVE (and copies), > > > otherwise it keeps using LW_SHARED as it does now (and does not copy.) > > > > Initially, I've considered copying statusFlags in > > ProcArrayInstallRestoredXmin() but I hesitated to do that because > > statusFlags is not relevant with xmin and snapshot stuff. But I agree > > that copying statusFlags should happen before restoring the snapshot. > > > > If we copy statusFlags in ProcArrayInstallRestoredXmin() there is > > still little window that the restored snapshot holds back the oldest > > xmin? > > That's wrong, I'd misunderstood. > > I agree to copy statusFlags in ProcArrayInstallRestoredXmin(). I've > updated the patch accordingly. I've tested this patch, and it correctly fixes the issue of blocking xmin from advancing, and also fixes an issue of retreating the observed *_oldest_nonremovable in XidHorizons through parallel workers. There are still some other soundness issues with xmin handling (see [0]), but that should not prevent this patch from landing in the relevant branches. Kind regards, Matthias van de Meent [0] https://www.postgresql.org/message-id/flat/17257-1e46de26bec11433%40postgresql.org
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.
On Fri, Nov 5, 2021 at 8:16 PM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote: > > On Fri, 22 Oct 2021 at 07:38, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 9:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > On Wed, Oct 20, 2021 at 3:07 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > > > > > On 2021-Oct-19, Alvaro Herrera wrote: > > > > > > > > > > Thank you for the comment. > > > > > > > > Hmm, I think this should happen before the transaction snapshot is > > > > > established in the worker; perhaps immediately after calling > > > > > StartParallelWorkerTransaction(), or anyway not after > > > > > SetTransactionSnapshot. In fact, since SetTransactionSnapshot receives > > > > > a 'sourceproc' argument, why not do it exactly there? ISTM that > > > > > ProcArrayInstallRestoredXmin() is where this should happen. > > > > > > > > ... and there is a question about the lock strength used for > > > > ProcArrayLock. The current routine uses LW_SHARED, but there's no > > > > clarity that we can modify proc->statusFlags and ProcGlobal->statusFlags > > > > without LW_EXCLUSIVE. > > > > > > > > Maybe we can change ProcArrayInstallRestoredXmin so that if it sees that > > > > proc->statusFlags is not zero, then it grabs LW_EXCLUSIVE (and copies), > > > > otherwise it keeps using LW_SHARED as it does now (and does not copy.) > > > > > > Initially, I've considered copying statusFlags in > > > ProcArrayInstallRestoredXmin() but I hesitated to do that because > > > statusFlags is not relevant with xmin and snapshot stuff. But I agree > > > that copying statusFlags should happen before restoring the snapshot. > > > > > > If we copy statusFlags in ProcArrayInstallRestoredXmin() there is > > > still little window that the restored snapshot holds back the oldest > > > xmin? > > > > That's wrong, I'd misunderstood. > > > > I agree to copy statusFlags in ProcArrayInstallRestoredXmin(). I've > > updated the patch accordingly. > > I've tested this patch, and it correctly fixes the issue of blocking > xmin from advancing, and also fixes an issue of retreating the > observed *_oldest_nonremovable in XidHorizons through parallel > workers. > > There are still some other soundness issues with xmin handling (see > [0]), but that should not prevent this patch from landing in the > relevant branches. > AFAICU, in the thread referred by you, it seems that the main reported issue will be resolved by this patch but there is a discussion about xmin moving backward which seems to be the case with the current code as per code comments mentioned by Andres. Is my understanding correct? -- With Regards, Amit Kapila.
On Wed, 10 Nov 2021 at 11:51, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Nov 5, 2021 at 8:16 PM Matthias van de Meent > <boekewurm+postgres@gmail.com> wrote: > > AFAICU, in the thread referred by you, it seems that the main reported > issue will be resolved by this patch but there is a discussion about > xmin moving backward which seems to be the case with the current code > as per code comments mentioned by Andres. Is my understanding correct? That is correct.
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/
Hi,
On 2021-11-11 12:22:42 +0900, Masahiko Sawada wrote:
> > 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?
This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin()
only happens in the context of much more expensive operations.
I think it might be worth asserting that the set of flags we're copying is a
known subset of the flags that are valid to copy from the source.
Greetings,
Andres Freund
On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-11-11 12:22:42 +0900, Masahiko Sawada wrote:
> > > 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?
>
> This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin()
> only happens in the context of much more expensive operations.
>
Fair point. I think that will also make the change in
ProcArrayInstallRestoredXmin() appear neat.
> I think it might be worth asserting that the set of flags we're copying is a
> known subset of the flags that are valid to copy from the source.
>
Sounds reasonable.
--
With Regards,
Amit Kapila.
On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2021-11-11 12:22:42 +0900, Masahiko Sawada wrote:
> > > > 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?
> >
> > This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin()
> > only happens in the context of much more expensive operations.
> >
>
> Fair point. I think that will also make the change in
> ProcArrayInstallRestoredXmin() appear neat.
Agreed.
This makes me think that it'd be better to copy status flags in a
separate function rather than ProcArrayInstallRestoredXmin(). The
current patch makes use of the fact that ProcArrayInstallRestoedXmin()
acquires a shared lock in order to check the source's status flags.
But if we can acquire an exclusive lock unconditionally in this
context, it’s clearer to do in a separate function.
>
> > I think it might be worth asserting that the set of flags we're copying is a
> > known subset of the flags that are valid to copy from the source.
> >
>
> Sounds reasonable.
+1
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Thu, Nov 11, 2021 at 10:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Hi,
> > >
> > > On 2021-11-11 12:22:42 +0900, Masahiko Sawada wrote:
> > > > > 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?
> > >
> > > This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin()
> > > only happens in the context of much more expensive operations.
> > >
> >
> > Fair point. I think that will also make the change in
> > ProcArrayInstallRestoredXmin() appear neat.
>
> Agreed.
>
> This makes me think that it'd be better to copy status flags in a
> separate function rather than ProcArrayInstallRestoredXmin(). The
> current patch makes use of the fact that ProcArrayInstallRestoedXmin()
> acquires a shared lock in order to check the source's status flags.
> But if we can acquire an exclusive lock unconditionally in this
> context, it’s clearer to do in a separate function.
>
Do you mean to say that do it in a separate function and call
immediately after StartParallelWorkerTransaction or do you mean to do
it in a separate function and invoke it from
ProcArrayInstallRestoedXmin()? I think the disadvantage I see by not
doing in ProcArrayInstallRestoedXmin is that we need to take procarray
lock twice (once in exclusive mode and then in shared mode) so doing
it in ProcArrayInstallRestoedXmin is beneficial from that angle. The
main reason why I was not very happy with the last patch was due to
releasing and reacquiring the lock but if we directly acquire it in
exclusive mode then that shouldn't be a problem. OTOH, doing it via a
separate function is also not that bad.
> >
> > > I think it might be worth asserting that the set of flags we're copying is a
> > > known subset of the flags that are valid to copy from the source.
> > >
> >
> > Sounds reasonable.
>
> +1
>
> Regards,
>
> --
> Masahiko Sawada
> EDB: https://www.enterprisedb.com/
--
With Regards,
Amit Kapila.
On Thu, Nov 11, 2021 at 3:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 10:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 2021-11-11 12:22:42 +0900, Masahiko Sawada wrote:
> > > > > > 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?
> > > >
> > > > This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin()
> > > > only happens in the context of much more expensive operations.
> > > >
> > >
> > > Fair point. I think that will also make the change in
> > > ProcArrayInstallRestoredXmin() appear neat.
> >
> > Agreed.
> >
> > This makes me think that it'd be better to copy status flags in a
> > separate function rather than ProcArrayInstallRestoredXmin(). The
> > current patch makes use of the fact that ProcArrayInstallRestoedXmin()
> > acquires a shared lock in order to check the source's status flags.
> > But if we can acquire an exclusive lock unconditionally in this
> > context, it’s clearer to do in a separate function.
> >
>
> Do you mean to say that do it in a separate function and call
> immediately after StartParallelWorkerTransaction or do you mean to do
> it in a separate function and invoke it from
> ProcArrayInstallRestoedXmin()?
I meant the former.
> I think the disadvantage I see by not
> doing in ProcArrayInstallRestoedXmin is that we need to take procarray
> lock twice (once in exclusive mode and then in shared mode) so doing
> it in ProcArrayInstallRestoedXmin is beneficial from that angle.
Right. I thought that this overhead is also negligible in this
context. If that’s right, it’d be better to do it in a separate
function from the clearness point of view. Also if we raise the lock
level in ProcArrayInstallRestoredXmin(), a caller of the function who
wants just to set xmin will end up acquiring an exclusive lock. Which
is unnecessary for the caller.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Thu, Nov 11, 2021 at 1:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Thu, Nov 11, 2021 at 3:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > I think the disadvantage I see by not > > doing in ProcArrayInstallRestoedXmin is that we need to take procarray > > lock twice (once in exclusive mode and then in shared mode) so doing > > it in ProcArrayInstallRestoedXmin is beneficial from that angle. > > Right. I thought that this overhead is also negligible in this > context. If that’s right, it’d be better to do it in a separate > function from the clearness point of view. Also if we raise the lock > level in ProcArrayInstallRestoredXmin(), a caller of the function who > wants just to set xmin will end up acquiring an exclusive lock. Which > is unnecessary for the caller. > As mentioned by Andres, ProcArrayInstallRestoredXmin() happens in an expensive context apart from this which is while creating logical replication, so the cost might not matter but I see your point about clarity. Basically, this function can get called from two different code paths i.e creation of logical replication slot and parallel worker startup but as of today we need it only in the latter case, so it is better to it in that code path (after calling StartParallelWorkerTransaction()). I think we can do that way unless Alvaro thinks otherwise as he had proposed to do it in ProcArrayInstallRestoredXmin(). Alvaro, others, do you favor any particular way to deal with this case? -- With Regards, Amit Kapila.
On 2021-Nov-11, Masahiko Sawada wrote: > On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote: > > > This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin() > > > only happens in the context of much more expensive operations. > > > > Fair point. I think that will also make the change in > > ProcArrayInstallRestoredXmin() appear neat. > > This makes me think that it'd be better to copy status flags in a > separate function rather than ProcArrayInstallRestoredXmin(). To me, and this is perhaps just personal opinion, it seems conceptually simpler to have ProcArrayInstallRestoredXmin acquire exclusive and do both things. Why? Because if you have two functions, you have to be careful not to call the new function after ProcArrayInstallRestoredXmin; otherwise you would create an instant during which you make an Xmin-without-flag visible to other procs; this causes the computed xmin go backwards, which is verboten. If I understand Amit correctly, his point is about the callers of RestoreTransactionSnapshot, which are two: CreateReplicationSlot and ParallelWorkerMain. He wants you hypothetical new function called from the latter but not the former. Looking at both, it seems a bit strange to make them responsible for a detail such as "copy ->statusFlags from source proc to mine". It seems more reasonable to add a third flag to RestoreTransactionSnapshot(Snapshot snapshot, void *source_proc, bool is_vacuum) and if that is true, tell SetTransactionSnapshot to copy flags, otherwise not. ... unrelated to this, and looking at CreateReplicationSlot, I wonder why does it pass MyProc as the source_pgproc parameter. What good is that doing? I mean, if the only thing we do with source_pgproc is to copy stuff from source_pgproc to MyProc, then if source_pgproc is MyProc, we're effectively doing nothing at all. (You can't "fix" this by merely passing NULL, because what that would do is change the calling of ProcArrayInstallRestoredXmin into a call of ProcArrayInstallImportedXmin and that would presumably have different behavior.) I may be misreading the code of course, but it sounds like the intention of CreateReplicationSlot is to "do nothing" with the transaction snapshot in a complicated way. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Fri, Nov 12, 2021 at 6:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Nov-11, Masahiko Sawada wrote: > > > On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote: > > > > > This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin() > > > > only happens in the context of much more expensive operations. > > > > > > Fair point. I think that will also make the change in > > > ProcArrayInstallRestoredXmin() appear neat. > > > > This makes me think that it'd be better to copy status flags in a > > separate function rather than ProcArrayInstallRestoredXmin(). > > To me, and this is perhaps just personal opinion, it seems conceptually > simpler to have ProcArrayInstallRestoredXmin acquire exclusive and do > both things. Why? Because if you have two functions, you have to be > careful not to call the new function after ProcArrayInstallRestoredXmin; > otherwise you would create an instant during which you make an > Xmin-without-flag visible to other procs; this causes the computed xmin > go backwards, which is verboten. > > If I understand Amit correctly, his point is about the callers of > RestoreTransactionSnapshot, which are two: CreateReplicationSlot and > ParallelWorkerMain. He wants you hypothetical new function called from > the latter but not the former. Looking at both, it seems a bit strange > to make them responsible for a detail such as "copy ->statusFlags from > source proc to mine". It seems more reasonable to add a third flag to > RestoreTransactionSnapshot(Snapshot snapshot, void *source_proc, bool is_vacuum) > and if that is true, tell SetTransactionSnapshot to copy flags, > otherwise not. > If we decide to go this way then I suggest adding a comment to convey why we choose to copy status flags in ProcArrayInstallRestoredXmin() as the function name doesn't indicate it. > > ... unrelated to this, and looking at CreateReplicationSlot, I wonder > why does it pass MyProc as the source_pgproc parameter. What good is > that doing? I mean, if the only thing we do with source_pgproc is to > copy stuff from source_pgproc to MyProc, then if source_pgproc is > MyProc, we're effectively doing nothing at all. (You can't "fix" this > by merely passing NULL, because what that would do is change the calling > of ProcArrayInstallRestoredXmin into a call of > ProcArrayInstallImportedXmin and that would presumably have different > behavior.) I may be misreading the code of course, but it sounds like > the intention of CreateReplicationSlot is to "do nothing" with the > transaction snapshot in a complicated way. > It ensures that the source transaction is still running, otherwise, it won't allow the import to be successful. It also seems to help by updating the state for GlobalVis* stuff. I think in the current form it seems to help in not moving MyProc-xmin and TransactionXmin backward due to checks in ProcArrayInstallRestoredXmin() and also change them to the value in source snapshot. -- With Regards, Amit Kapila.
On Sat, Nov 13, 2021 at 2:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Nov 12, 2021 at 6:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2021-Nov-11, Masahiko Sawada wrote: > > > > > On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote: > > > > > > > This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin() > > > > > only happens in the context of much more expensive operations. > > > > > > > > Fair point. I think that will also make the change in > > > > ProcArrayInstallRestoredXmin() appear neat. > > > > > > This makes me think that it'd be better to copy status flags in a > > > separate function rather than ProcArrayInstallRestoredXmin(). > > Thank you for the comment! > > To me, and this is perhaps just personal opinion, it seems conceptually > > simpler to have ProcArrayInstallRestoredXmin acquire exclusive and do > > both things. Why? Because if you have two functions, you have to be > > careful not to call the new function after ProcArrayInstallRestoredXmin; > > otherwise you would create an instant during which you make an > > Xmin-without-flag visible to other procs; this causes the computed xmin > > go backwards, which is verboten. I agree that it's simpler. I thought statusFlags and xmin are conceptually separate things since PROC_VACUUM_FOR_WRAPAROUND is not related to xid at all for example. But given that the use case of copying statusFlags from someone is only parallel worker startup for now, copying statusFlags while setting xmin seems convenient and simple. If we want to only copy statusFlags in some use cases in the future, we can have a separate function for that. > > > > If I understand Amit correctly, his point is about the callers of > > RestoreTransactionSnapshot, which are two: CreateReplicationSlot and > > ParallelWorkerMain. He wants you hypothetical new function called from > > the latter but not the former. Looking at both, it seems a bit strange > > to make them responsible for a detail such as "copy ->statusFlags from > > source proc to mine". It seems more reasonable to add a third flag to > > RestoreTransactionSnapshot(Snapshot snapshot, void *source_proc, bool is_vacuum) > > and if that is true, tell SetTransactionSnapshot to copy flags, > > otherwise not. For the idea of is_vacuum flag, we don't know if a parallel worker is launched for parallel vacuum at the time of ParallelWorkerMain(). > > > > If we decide to go this way then I suggest adding a comment to convey > why we choose to copy status flags in ProcArrayInstallRestoredXmin() > as the function name doesn't indicate it. Agreed. I've updated the patch so that ProcArrayInstallRestoredXmin() sets both xmin and statusFlags only when the source proc is still running and xmin doesn't go backwards. IOW it doesn't happen that only one of them is set by this function, which seems more understandable behavior. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
On Mon, Nov 15, 2021 at 12:38 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > I've updated the patch so that ProcArrayInstallRestoredXmin() sets > both xmin and statusFlags only when the source proc is still running > and xmin doesn't go backwards. IOW it doesn't happen that only one of > them is set by this function, which seems more understandable > behavior. > How have you tested this patch? As there was no test case presented in this thread, I used the below manual test to verify that the patch works. The idea is to generate a scenario where a parallel vacuum worker holds back the xmin from advancing. Setup: -- keep autovacuum = off in postgresql.conf create table t1(c1 int, c2 int); insert into t1 values(generate_series(1,1000),100); create index idx_t1_c1 on t1(c1); create index idx_t1_c2 on t1(c2); create table t2(c1 int, c2 int); insert into t2 values(generate_series(1,1000),100); create index idx_t2_c1 on t1(c1); Session-1: delete from t1 where c1 < 10; --this is to ensure that vacuum has some work to do Session-2: -- this is done just to ensure the Session-1's xmin captures the value of this xact begin; select txid_current(); -- say value is 725 insert into t2 values(1001, 100); Session-1: set min_parallel_index_scan_size=0; -- attach a debugger and ensure to stop parallel worker somewhere before it completes and the leader after launching parallel worker vacuum t1; Session-2: -- commit the open transaction commit; Session-3: -- attach a debugger and break at the caller of vacuum_set_xid_limits. vacuum t2; I noticed that before the patch the value of oldestXmin in Session-3 is 725 but after the patch it got advanced. I have made minor edits to the attached patch. See, if this looks okay to you then please prepare and test the patch for back-branches as well. If you have some other way to test the patch then do share the same and let me know if you see any flaw in the above verification method. -- With Regards, Amit Kapila.
Attachment
On Tue, Nov 16, 2021 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Nov 15, 2021 at 12:38 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > I've updated the patch so that ProcArrayInstallRestoredXmin() sets > > both xmin and statusFlags only when the source proc is still running > > and xmin doesn't go backwards. IOW it doesn't happen that only one of > > them is set by this function, which seems more understandable > > behavior. > > > > How have you tested this patch? As there was no test case presented in > this thread, I used the below manual test to verify that the patch > works. The idea is to generate a scenario where a parallel vacuum > worker holds back the xmin from advancing. > > Setup: > -- keep autovacuum = off in postgresql.conf > create table t1(c1 int, c2 int); > insert into t1 values(generate_series(1,1000),100); > create index idx_t1_c1 on t1(c1); > create index idx_t1_c2 on t1(c2); > > create table t2(c1 int, c2 int); > insert into t2 values(generate_series(1,1000),100); > create index idx_t2_c1 on t1(c1); > > Session-1: > delete from t1 where c1 < 10; --this is to ensure that vacuum has some > work to do > > Session-2: > -- this is done just to ensure the Session-1's xmin captures the value > of this xact > begin; > select txid_current(); -- say value is 725 > insert into t2 values(1001, 100); > > Session-1: > set min_parallel_index_scan_size=0; > -- attach a debugger and ensure to stop parallel worker somewhere > before it completes and the leader after launching parallel worker > vacuum t1; > > Session-2: > -- commit the open transaction > commit; > > Session-3: > -- attach a debugger and break at the caller of vacuum_set_xid_limits. > vacuum t2; Yes, I've tested this patch in a similar way; while running pgbench in the background in order to constantly consume XID, I checked if the oldest xmin in VACUUM VERBOSE log is advancing even during parallel vacuum running. > > I noticed that before the patch the value of oldestXmin in Session-3 > is 725 but after the patch it got advanced. I have made minor edits to > the attached patch. See, if this looks okay to you then please prepare > and test the patch for back-branches as well. If you have some other > way to test the patch then do share the same and let me know if you > see any flaw in the above verification method. The patch looks good to me. But I can't come up with a stable test for this. It seems to be hard without stopping and resuming parallel vacuum workers. Do you have any good idea? I've attached patches for back branches (13 and 14). Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
On Wed, Nov 17, 2021 at 1:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Tue, Nov 16, 2021 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > The patch looks good to me. But I can't come up with a stable test for > this. It seems to be hard without stopping and resuming parallel > vacuum workers. Do you have any good idea? > No, let's wait for a day or so to see if anybody else has any ideas to write a test for this case, otherwise, I'll check these once again and push. -- With Regards, Amit Kapila.
On Wed, Nov 17, 2021 at 7:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > The patch looks good to me. But I can't come up with a stable test for
> > this. It seems to be hard without stopping and resuming parallel
> > vacuum workers. Do you have any good idea?
> >
>
> No, let's wait for a day or so to see if anybody else has any ideas to
> write a test for this case, otherwise, I'll check these once again and
> push.
I set this "committed" in the CF app.
--
John Naylor
EDB: http://www.enterprisedb.com
On Wed, Nov 24, 2021 at 7:46 PM John Naylor <john.naylor@enterprisedb.com> wrote: > > On Wed, Nov 17, 2021 at 7:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > The patch looks good to me. But I can't come up with a stable test for > > > this. It seems to be hard without stopping and resuming parallel > > > vacuum workers. Do you have any good idea? > > > > > > > No, let's wait for a day or so to see if anybody else has any ideas to > > write a test for this case, otherwise, I'll check these once again and > > push. > > I set this "committed" in the CF app. > Thanks! -- With Regards, Amit Kapila.