Re: Parallel autovacuum: DROP DATABASE WITH (FORCE) fails on the parallel workers - Mailing list pgsql-hackers

On Mon, Oct 5, 2026 at 5:27 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Sep 29, 2026 at 7:46 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Hi,
> >
> > On Mon, Sep 28, 2026 at 11:39 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > Thank you for the report and the patch!
> >
> > Thanks for taking a look at it.
> >
> > > IIUC the issue stems from the fact that the leader and its workers
> > > advertise different roleIds (InvalidOid and BOOTSTRAP_SUPERUSERID). I
> > > think the same issue can be reproduced in other cases. For instance,
> > > suppose that a bgworker connecting to the database via
> > > BackgroundWorkerInitializeConnection(dbname, NULL, 0) runs a parallel
> > > query, the leader's roleId is InvalidOid whereas the parallel query
> > > workers have BOOTSTRAP_SUPERUSERID. I think we should fix it as well
> > > and backpatch the fix to 14.
> >
> > Good catch. Yes, it happens there too. I verified it locally with a
> > simple test module that launches a background worker and starts a
> > parallel worker to run a query. I agree the fix needs to be
> > back-patched through 14.
> >
> > > I have some review comments on the proposed patch:
> > >
> > > +               if (leader != NULL && leader != proc &&
> > > +                   leader->backendType == B_AUTOVAC_WORKER)
> > > +                   roleId = InvalidOid;
> > >
> > > I think we should check a lock group member with its leader's roleId
> > > instead of unconditionally using InvalidOid. That would
> > > straightforwardly fix the inconsistency between the leader and the
> > > workers.
> > >
> > >     if (leader != NULL && leader != proc &&
> > >         leader->databaseId == databaseId)
> > >         roleId = leader->roleId;
> > >
> > > To fix this issue not only in autovacuum cases, the backendType check
> > > should be removed.
> >
> > Agreed, that is better. Checking a parallel worker against its
> > leader's roleId is the right choice here, since both are members of
> > the same lock group. It gives the expected behavior, terminating both
> > the leader and the workers for DROP DATABASE FORCE, which both run as
> > the bootstrap superuser.
> >
> > Please find the attached v2 patch. I plan to share patches for the
> > back branches without the TAP test for branches < PG19 unless there
> > are any comments.
>
> I've reviewed the patch and have one comment:
>
> +               PGPROC     *leader = proc->lockGroupLeader;
> +               Oid         roleId = proc->roleId;
> +
> ...
> +                */
> +               if (leader != NULL && leader != proc &&
> +                   leader->databaseId == databaseId)
> +                   roleId = leader->roleId;
>
> We read another backend's lockGroupLeader and dereference it without
> holding the lock that protects the lock group fields, which seems
> incorrect to me. If the worker exits after we read the pointer, the
> leader's PGPROC could be recycled and we could check an unrelated
> backend's role. I think that we can acquire the leader's partition
> lock and recheck if the leader is still the leader we're looking for,
> like BecomeLockGroupMember() does. It would require one more LWLock
> acquisition per lock group member but probably we can live with it as
> DROP DATABASE FORCE isn't a hot path. What do you think?

On second thought, I think we don't need to acquire the leader's
partition lock here. The leader's PGPROC can be recycled only after
the last member has exited, so if we read an unrelated backend's role
that way, the worker we're checking has already gone. That's no
different from the existing race mentioned in the comment in
TerminateOtherDBBackends(), where a process can exit before we send
the signal. Acquiring the lock would not close that race either. So I
think it's enough to add a comment explaining why it's okay to read
these fields without the lock. Sorry for the noise.

Also, I think it's better to add the !OidIsValid(leader->roleId) check
to the if statement (and set roleId = InvalidOid directly). That way,
the change would affect only the specific case where the leader
doesn't advertise its roleId while its workers do, which seems better
to me for backpatching than creating a generic rule "if the leader and
the worker have different roles, the worker inherits the leader's
roleId only when being terminated".

For the record, I've considered alternative solutions but the current
approach is reasonable to me.

A. Set parallel autovacuum worker's roleId to InvalidOid at its
startup time. While it's good that it affects only parallel autovacuum
cases, it would add another place to set the process's roleId. Also,
there is still a small window where its roleId is
BOOTSTRAP_SUPERUSERID. To remove the window completely, we need to
 change parallel query code so that workers advertise the same roleId
as their leader, which is too invasive.

B. Ignore parallel workers in TerminateOtherDBBackends(). I think it's
the second-best idea but I'm concerned that it would affect the
existing behavior that terminates parallel workers.

When it comes to the current idea, it changes
TerminateOtherDBBackends() but the change would affect only the cases
where the leader doesn't advertise its roleId, i.e., parallel
autovacuum and parallel queries run by a bgworker connected without a
user, and doesn't change the existing behavior of terminating parallel
workers.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com