Hi JoongHyuk,
I tested v2 (both patches) on top of master 9f191dce61e, built with
--enable-cassert --enable-debug --enable-tap-tests. Both patches apply
in order with git apply and build without warnings. These TAP tests pass:
0001: src/test/recovery t/003_recovery_targets.pl (33 tests)
0001+0002: t/003_recovery_targets.pl and t/005_replay_delay.pl
(37 tests)
I also ran PITR by hand: a primary with archive_mode, a base backup,
then DROP DATABASE, CREATE DATABASE (STRATEGY file_copy), CREATE
TABLESPACE, DROP TABLESPACE and ALTER DATABASE SET TABLESPACE, each
preceded by a recorded time, LSN and restore point. I recovered every
case with recovery_target_time (inclusive on and off), LSN (on and
off), name and xid, on master and with 0001, and looked at the
catalog and at base/ and pg_tblspc/ after promotion.
What works well:
- On master, a time target just before the command ends in a broken
state, exactly as you describe. DROP DATABASE: dbx is in the catalog
but its directory is gone ("cannot connect to invalid database").
CREATE DATABASE leaves an orphan base/<oid>. CREATE TABLESPACE leaves
an orphan pg_tblspc/<oid>. DROP TABLESPACE leaves a tablespace in
pg_tablespace with no directory.
- With 0001, the same targets stop with "recovery stopping before
recovery boundary" and the cluster is clean in all four cases, for
both inclusive on and off.
- LSN and name targets behave the same as on master.
- pg_waldump shows the new record as "XLOG2 ... RECOVERY_BOUNDARY time
...", and the record sits just before the first non-transactional
record of each command.
- xid target: your explanation matches what I see. With the xid of
DROP DATABASE and inclusive = off I get the same "cannot connect to
invalid database", and with the xid of the previous transaction and
inclusive = on the database is intact.
Points I found:
1. A time target between the boundary record and the commit record
still breaks the database. The boundary time is taken when the
command starts, but the commit comes later, and CREATE DATABASE and
DROP DATABASE wait for checkpoints in between. In my runs:
- DROP DATABASE, target = boundary time + 1 microsecond, inclusive
on: "recovery stopping before commit of transaction 672", dbx is
in the catalog, its directory is missing, and connecting gives
"cannot connect to invalid database".
- CREATE DATABASE (file_copy), target halfway between the boundary
(13:43:49.99) and the commit (13:43:51.01), inclusive on or off:
recovery stops before the commit and base/16397 is left as an
orphan directory.
I do not see an easy code fix, because recovery only learns about
the commit later. Maybe the docs for recovery_target_time could
say that a target taken while such a command is running can still
leave this result.
2. With recovery_target_inclusive = on and a target exactly equal to
the boundary time, recovery does not stop at the boundary (the test
is boundary_time > target), replays the file operation, and then
stops before the commit because the commit time is later. Result
for DROP DATABASE: dbx in the catalog, directory missing. With
inclusive = off the same target stops before the boundary and the
database is fine. Since the boundary record is not a transaction
that can be "included", maybe it should use >= for both settings.
This is only an edge case (microsecond equality), but the two
settings give different results for the same WAL.
3. 0001 does not touch the documentation. The recovery_target_time
and recovery_target_inclusive text in config.sgml (or the PITR
chapter) does not mention that recovery now also stops before these
commands, nor that xid and LSN targets are not covered (the xid
case Shihao reported). Even one sentence with the workaround you
gave (use the xid of the previous transaction with inclusive = on)
would help operators. 0002 does update the recovery_min_apply_delay
paragraph.
4. Small thing about messages. "recovery stopping before recovery
boundary, time ..." and the pg_waldump line do not say which
command the boundary belongs to, so an operator reading the log
cannot tell if it was a DROP DATABASE or a CREATE TABLESPACE
without looking at the next WAL record. Not important, only a
suggestion (for example a one-byte command type in the record).
For 0002 I only ran the TAP test (005_replay_delay.pl passes with the
new case); I did not test it by hand.
Thanks for working on this. Losing a database or getting orphan
directories after PITR is a real problem.
Regards,
João Rito
Em qua., 11 de nov. de 2026 às 11:18, JoongHyuk Shin
<[email protected]> escreveu:
>
> Hi Shihao,
>
> Thanks for the review. I should have described the problem in more
> detail in my first message.
>
> For now, I don't plan to handle the xid case in this patch, in code or
> in the docs. With the xid of DROP DATABASE and
> recovery_target_inclusive = off, recovery stops right before its commit
> record, as documented, but the changes DROP DATABASE makes before
> committing have been replayed by then. Recovery knows it has reached an
> xid target when it reads that transaction's commit record, so the
> operator can instead give the xid of a transaction known to have
> committed before DROP DATABASE started, with
> recovery_target_inclusive = on. Whether the docs should say this is a
> separate question. It is not specific to this patch or to xid targets,
> as an LSN target between those changes and the commit, with
> recovery_target_inclusive = off, leaves the same result on master.
>
> But a time target is different. Recovery knows it has passed a time
> target only when it reads a later timestamp, which on master comes only
> from commit and abort records, so the first one can be the commit of
> DROP DATABASE itself, after its changes have been replayed. The
> boundary record this patch adds gives recovery a timestamp ahead of
> those changes.
>
> v2 is only a rebase onto current master.
>
> --
> JoongHyuk Shin
