Arvados

Can this be changed so that the TrashSweep only takes the lock before starting the sweep and frees it once finished?

I don't see a reason to hold a lock for the lifetime of the controller when it's only supposed to prevent multiple TrashSweep processes from running concurrently.

Holding a lock keeps a transaction opened which affects the behaviour of VACUUM in PostgreSQL and prevents it from freeing data deleted after the lock was acquired.

The purpose of this lock isn't only to prevent concurrent trash sweeps, it's to ensure only one controller process does periodic trash sweeps. E.g., if trash sweep interval is 5 minutes and there are 5 controller processes/hosts, we want 1 trash sweep every 5 minutes, not 5.

We are planning to use a similar long-lived lock to ensure only one keep-balance process runs at a time, etc. If the PostgreSQL feature does not work as a long-lived lock, we'll need to find something else. But it seems a bit odd that the advisory lock feature would have such a significant side effect that (afaics) isn't mentioned in the docs.

Re keeping a transaction open, this does not use a transaction, and it isn't associated with any table, so I don't think it would prevent VACUUM. FWIW I tried it in pg console and "lock table" blocked a vacuum but pg_advisory_lock() did not.

You can see from the output of the following queries that the vacuum is blocked by oldest xmin: 129621260 which corresponds to advisory_lock

It could be that the problem is the "pending" lock and not the acquired lock.

arvados_production=> select pid, age(clock_timestamp(), query_start), state, usename, backend_xid, backend_xmin, query from pg_stat_activity WHERE query != '<IDLE>' AND query NOT ILIKE '%pg_stat_activity%' AND state!='idle';
  pid  |          age          | state  | usename | backend_xid | backend_xmin |            query
-------+-----------------------+--------+---------+-------------+--------------+-----------------------------
 17088 | 1 day 16:26:51.612997 | active | arvados |             |    129621260 | SELECT pg_advisory_lock($1)
(1 row)

arvados_production=> vacuum verbose materialized_permissions ;
INFO:  vacuuming "public.materialized_permissions" 
INFO:  launched 2 parallel vacuum workers for index cleanup (planned: 2)
INFO:  "materialized_permissions": found 0 removable, 1196424 nonremovable row versions in 15145 out of 35269 pages
DETAIL:  1193 dead row versions cannot be removed yet, oldest xmin: 129621260
...

Yes, it seems plausible that a waiting pg_advisory_lock() statement counts as an open transaction for vacuum purposes (after all, under the hood a single statement running outside a transaction is really a single-statement transaction) but vacuum is unaffected by the fact of an advisory lock being held in an open session.

In that case, the change in note-4 would resolve the issue.

Regardless, we discussed this relative to our longer term plans and decided we should use a different locking strategy anyway and we plan to have a new implementation in 2.3.2 that does not use pg_advisory_lock at all.

Tom Clegg wrote:

18488-nonblocking-advisory-lock @ 9ed314b7a585970c03c87959286fc1e582d769f7 -- developer-run-tests: #2823

LGTM thanks.