Horton

Thread

Commits

  1. Fix parallel operations that prevent oldest xmin from advancing.

  1. Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-06T07:10:50Z

    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/

  2. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Nathan Bossart <bossartn@amazon.com> — 2021-10-06T16:43:05Z

    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

  3. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-07T05:56:14Z

    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.

  4. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-08T15:13:25Z

    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)

  5. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Peter Geoghegan <pg@bowt.ie> — 2021-10-08T17:21:31Z

    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

  6. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-11T00:23:32Z

    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/

  7. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Michael Paquier <michael@paquier.xyz> — 2021-10-11T00:50:54Z

    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

  8. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Greg Nancarrow <gregn4422@gmail.com> — 2021-10-11T06:01:22Z

    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

  9. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-12T00:25:46Z

    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/

  10. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-12T06:53:31Z

    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/

  11. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-19T15:35:56Z

    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/

  12. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-19T18:07:17Z

    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/

  13. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-20T00:27:40Z

    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/

  14. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-22T05:38:02Z

    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/

  15. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-11-05T14:45:51Z

    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

  16. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-10T09:14:00Z

    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.

  17. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-10T10:51:30Z

    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.

  18. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-11-10T12:11:56Z

    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.

  19. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-11T03:22:42Z

    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/

  20. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Andres Freund <andres@anarazel.de> — 2021-11-11T03:41:53Z

    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

  21. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-11T03:53:22Z

    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.

  22. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-11T05:09:42Z

    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/

  23. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-11T06:07:20Z

    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.

  24. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-11T08:06:31Z

    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/

  25. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-12T03:43:59Z

    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.

  26. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-12T13:14:11Z

    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/

  27. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-13T05:10:30Z

    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.

  28. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-15T07:08:00Z

    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/

  29. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-16T11:45:21Z

    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.

  30. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-17T07:35:47Z

    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/

  31. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-17T11:27:42Z

    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.

  32. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    John Naylor <john.naylor@enterprisedb.com> — 2021-11-24T14:15:52Z

    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

  33. Re: Parallel vacuum workers prevent the oldest xmin from advancing

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-25T03:30:20Z

    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.