Fix possible two vacuums concurrently processing the same relfilenode.

[MasahikoSawada]

Previously, while swapping the target table and its temp table, we used to acquire an AccessExclusiveLock on only the target table but not on its temp table. So the following scenario could happen:

1. Run (auto) vacuum on the temp table.
2. Swap the target table's relfile node and the temp table's relfilenode, and commit.
3. Run (auto) vacuum on the target table.

1 and 3 run concurrently on different tables but these tables use the same relfilenode. In the reported case, since concurrent changes (INSERT/UPDATE/DELETE) can also run concurrently, the second vacuum ended up removing newly added tuples.

To fix the issue, this commit changes the pg_repack command so that it acquires an AcessExclusiveLock also on the temp table before swapping these tables.

Issue: #399

[Melkij]

Thanks! It looks like pg_reorg originally relied on autovacuum=off , but that's really not enough (anti-wraparound autovacuum can still start). Explicit blocking is much more correct.
Will merge in a couple of days if there are no objections.

[MasahikoSawada]

Thank you for looking at the fix.

I guess we might want to check in repack_swap() if two relations are locked in AccessExclusiveMode for sure, for example by using CheckRelationLockedByMe(). What do you think?

[Melkij]

sounds like good check. Although for now it should be conditional #if PG_VERSION_NUM >= 120000. CheckRelationLockedByMe does not exist in all versions we still support.

[za-arthur]

Thank you for the PR!

Would it make sense to assign 0 to temp_oid after initializing repack_table?

pg_repack/bin/pg_repack.c

Line 902 in f5aa388

repack_table table;

I guess we might want to check in repack_swap() if two relations are locked in AccessExclusiveMode for sure, for example by using CheckRelationLockedByMe(). What do you think?

Indeed, it might be a good addition. I think instead Assert that check should raise a runtime error.

[MasahikoSawada]

Thank you for reviewing the change! I've pushed the changes.

I use LockHeldByMe() instead of CheckRelationLockedByMe() since we don't open relations. These checks are available only on > PG12.

[za-arthur]

Thanks for the fix! Looks good to me.

[MasahikoSawada]

Thank you for reviewing the change! I've merged it.