Re: standby recovery fails (tablespace related) (tentative patch and discussion)

On Wed, Apr 24, 2019 at 4:14 PM Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Mmm. I posted to wrong thread. Sorry.

At Tue, 23 Apr 2019 16:39:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190423.163949.36763221.horiguchi.kyotaro@lab.ntt.co.jp>
> At Tue, 23 Apr 2019 13:31:58 +0800, Paul Guo <pguo@pivotal.io> wrote in <CAEET0ZEcwz57z2yfWRds43b3TfQPPDSWmbjGmD43xRxLT41NDg@mail.gmail.com>
> > Hi Kyotaro, ignoring the MakePGDirectory() failure will fix this database
> > create redo error, but I suspect some other kind of redo, which depends on
> > the files under the directory (they are not copied since the directory is
> > not created) and also cannot be covered by the invalid page mechanism,
> > could fail. Thanks.
>
> If recovery starts from just after tablespace creation, that's
> simple. The Symlink to the removed tablespace is already removed
> in the case. Hence server innocently create files directly under
> pg_tblspc, not in the tablespace. Finally all files that were
> supposed to be created in the removed tablespace are removed
> later in recovery.
>
> If recovery starts from recalling page in a file that have been
> in the tablespace, XLogReadBufferExtended creates one (perhaps
> directly in pg_tblspc as described above) and the files are
> removed later in recoery the same way to above. This case doen't
> cause FATAL/PANIC during recovery even in master.
>
> XLogReadBufferExtended@xlogutils.c
> | * Create the target file if it doesn't already exist.  This lets us cope
> | * if the replay sequence contains writes to a relation that is later
> | * deleted.  (The original coding of this routine would instead suppress
> | * the writes, but that seems like it risks losing valuable data if the
> | * filesystem loses an inode during a crash.  Better to write the data
> | * until we are actually told to delete the file.)
>
> So buffered access cannot be a problem for the reason above. The
> remaining possible issue is non-buffered access to files in
> removed tablespaces. This is what I mentioned upthread:
>
> me> but I haven't checked this covers all places where need the same
> me> treatment.

RM_DBASE_ID is fixed by the patch.

XLOG/XACT/CLOG/MULTIXACT/RELMAP/STANDBY/COMMIT_TS/REPLORIGIN/LOGICALMSG:
  - are not relevant.

HEAP/HEAP2/BTREE/HASH/GIN/GIST/SEQ/SPGIST/BRIN/GENERIC:
  - Resources works on buffer is not affected.

SMGR:
  - Both CREATE and TRUNCATE seems fine.

TBLSPC:
  - We don't nest tablespace directories. No Problem.

I don't find a similar case.

I took some time in digging into the related code. It seems that ignoring if the dst directory cannot be created directly

should be fine since smgr redo code creates tablespace path finally by calling TablespaceCreateDbspace().

What's more, I found some more issues.

1) The below error message is actually misleading.

2019-04-17 14:52:14.951 CST [23030] FATAL:  could not create directory "pg_tblspc/65546/PG_12_201904072/65547": No such file or directory
2019-04-17 14:52:14.951 CST [23030] CONTEXT:  WAL redo at 0/3011650 for Database/CREATE: copy dir 1663/1 to 65546/65547

That should be due to dbase_desc(). It could be simply fixed following the code logic in GetDatabasePath().

2) It seems that src directory could be missing then dbase_redo()->copydir() could error out. For example,

\!rm -rf /tmp/tbspace1  
\!mkdir /tmp/tbspace1
\!rm -rf /tmp/tbspace2
\!mkdir /tmp/tbspace2
create tablespace tbs1 location '/tmp/tbspace1'; 
create tablespace tbs2 location '/tmp/tbspace2'; 
create database db1 tablespace tbs1;
alter database db1 set tablespace tbs2;
drop tablespace tbs1;

Let's say, the standby finishes all replay but redo lsn on pg_control is still the point at 'alter database', and then

kill postgres, then in theory when startup, dbase_redo()->copydir() will ERROR since 'drop tablespace tbs1'

has removed the directories (and symlink) of tbs1. Below simple code change could fix that.

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c

index 9707afabd9..7d755c759e 100644

--- a/src/backend/commands/dbcommands.c

+++ b/src/backend/commands/dbcommands.c

@@ -2114,6 +2114,15 @@ dbase_redo(XLogReaderState *record)

         */

        FlushDatabaseBuffers(xlrec->src_db_id);

+       /*

+        * It is possible that the source directory is missing if

+        * we are re-replaying the xlog while subsequent xlogs

+        * drop the tablespace in previous replaying. For this

+        * we just skip.

+        */

+       if (!(stat(src_path, &st) == 0 && S_ISDIR(st.st_mode)))

+           return;

+

        /*

         * Copy this subdirectory to the new location

         *

If we want to fix the issue by ignoring the dst path create failure, I do not think we should do

that in copydir() since copydir() seems to be a common function. I'm not sure whether it is

used by some extensions or not. If no maybe we should move the dst patch create logic

out of copydir().

Also I'd suggest we should use pg_mkdir_p() in TablespaceCreateDbspace() to replace

the code block includes a lot of get_parent_directory(), MakePGDirectory(), etc even it

is not fixing a bug since pg_mkdir_p() code change seems to be more graceful and simpler.

 

Whatever ignore mkdir failure or mkdir_p, I found that these steps seem to be error-prone

along with postgre evolving since they are hard to test and also we are not easy to think out

various potential bad cases. Is it possible that we should do real checkpoint (flush & update

redo lsn) when seeing checkpoint xlogs for these operations? This will slow down standby

but master also does this and also these operations are not usual, espeically it seems that it

does not slow down wal receiving usually?