Re: REPACK (CONCURRENTLY) can lose data in pg_dump output

Hi,

> I realise REPACK (CONCURRENTLY) is documented as not being MVCC-safe,
> Should this at least be documented on the pg_dump page, or could [...]

Yeah, this is expected and not specifically introduced by REPACK. Any
command that rewrites the table will also store the rows with a new
xmin, so these rows are now not visible to a transaction that has a
snapshot that pre-dates the rewrite. pg_dump runs in REPEATABLE READ
isolation level and has a transaction snapshot, so it will not see the
rewritten rows. A query inside a STABLE/IMMUTABLE function even when run
through a READ COMMITTED transaction will also have the same behavior.

You can repro the same behavior with an ALTER TABLE...ALTER COLUMN:

```
postgres=# CREATE TABLE altertest (id int PRIMARY KEY, val int);
INSERT INTO altertest SELECT g, g FROM generate_series(1,1000) g;
CREATE TABLE
INSERT 0 1000
postgres=#
```

s1:
```
postgres=# BEGIN;
ALTER TABLE altertest ALTER COLUMN val TYPE bigint;
BEGIN
ALTER TABLE
postgres=*#
```

s2:
```
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
SELECT count(*) FROM altertest; ---> hangs as it waits on s1
BEGIN
```

s1:
```
COMMIT;
```

and now s2 goes through:

```
count
-------
0
(1 row)

postgres=*# SELECT count(*) FROM altertest;
count
-------
0
(1 row)

postgres=*# INSERT INTO altertest VALUES (1, 1);
ERROR: duplicate key value violates unique constraint "altertest_pkey"
DETAIL: Key (id)=(1) already exists.
postgres=!# ROLLBACK;
ROLLBACK
postgres=# SELECT count(*) FROM altertest;
count
-------
1000
(1 row)
```

s2 takes its snapshot before it starts waiting on s1, which is why it
still sees the table as empty after s1 commits.

Looking at the mvcc-caveats [1], I think for the most part this behavior
is described properly, but there may need to be a few points of
clarification:

1/ "which would block the truncating or rewriting command until that
transaction completes."

This is still true for REPACK CONCURRENTLY, but only because of the swap
phase, when it must take an exclusive lock; otherwise it runs with a
non-conflicting share update exclusive lock. That is probably worth
describing in this doc, since CONCURRENTLY is specifically meant to not
conflict with readers, at least for the most part.

2/ "So these commands will not cause any apparent inconsistency in the
table contents for successive queries on the target table, but they
could cause visible inconsistency between the contents of the target
table and other tables in the database."

This is true but does not actually state the whole story. If the rows are
rewritten, such as ALTER TABLE..ALTER COLUMN or REPACK, it should be
mentioned that the rewritten rows are stored with new transaction IDs and
that there could be inconsistencies even within the same table, as is the
case above where the SELECT shows 0 rows but we fail on a primary key
conflict on INSERT.

> Should this at least be documented on the pg_dump page

I don't think so. TRUNCATE, ALTER TABLE and REPACK all already reference
mvcc-caveats, and this is not specific to pg_dump. Any client that takes
its snapshot before locking what it reads can hit it, so it seems better
to keep it all in mvcc-caveats and maybe use pg_dump there as the example.

WDYT?

[1] https://www.postgresql.org/docs/19/mvcc-caveats.html

--
Sami Imseih
Amazon Web Services (AWS)