Hi Anastasia
>
> But during the review, I found a bug in the current implementation.
> New behavior must apply to crash-recovery only, now it applies to archiveRecovery too.
> That can cause a silent loss of a tablespace during regular standby operation
> since it never calls CheckRecoveryConsistency().
>
> Steps to reproduce:
> 1) run master and replica
> 2) create dir for tablespace:
> mkdir /tmp/tblspc1
>
> 3) create tablespace and database on the master:
> create tablespace tblspc1 location '/tmp/tblspc1';
> create database db1 tablespace tblspc1 ;
>
> 4) wait for replica to receive this changes and pause replication:
> select pg_wal_replay_pause();
>
> 5) move replica's tablespace symlink to some empty directory, i.e. /tmp/tblspc2
> mkdir /tmp/tblspc2
> ln -sfn /tmp/tblspc2 postgresql_data_replica/pg_tblspc/16384
>
By changing the tablespace symlink target, we are silently nullifying effects of a committed transaction from the standby data directory - the directory structure created by the standby for create tablespace transaction. This step, therefore, does not look like a valid test case to me. Can you share a sequence of steps that does not involve changing data directory manually?
>
> Also some nitpicking about code style:
> 1) Please, add comment to forget_missing_directory().
>
> 2) + elog(LOG, "Directory \"%s\" was missing during directory copying "
> I think we'd better update this message elevel to WARNING.
>
> 3) Shouldn't we also move FlushDatabaseBuffers(xlrec->src_db_id); call under
> if (do_copydir) clause?
> I don't see a reason to flush pages that we won't use later.
>
Thank you for the review feedback. I agree with all the points. Let me incorporate them (I plan to pick this work up and drive it to completion as Paul got busy with other things).
But before that I'm revisiting another solution upthread, that of creating restart points when replaying create/drop database commands before making filesystem changes such as removing a directory. The restart points should align with checkpoints on master. The concern against this solution was creation of restart points will slow down recovery. I don't think crash recovery is affected by this solution because of the already existing enforcement of checkpoints. WAL records prior to a create/drop database will not be seen by crash recovery due to the checkpoint enforced during the command's normal execution.
Asim