Horton

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

Paul Guo <guopa@vmware.com>

View thread View on postgr.es
From: Paul Guo <guopa@vmware.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Anastasia Lubennikova <a.lubennikova@postgrespro.ru>, "Asim Praveen (Pivotal)" <apraveen@pivotal.io>, "Lei Wang (Pivotal)" <leiwang@pivotal.io>, Thomas Munro <thomas.munro@gmail.com>, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Date: 2020-07-08T12:56:44Z
Lists: pgsql-hackers

Attachments

Looks like my previous reply was held for moderation (maybe due to my new email address).
I configured my pg account today using the new email address. I guess this email would be
held for moderation.

I’m now replying my previous reply email and attaching the new patch series.


On Jul 6, 2020, at 10:18 AM, Paul Guo <guopa@vmware.com<mailto:guopa@vmware.com>> wrote:

Thanks for the review. I’m now re-picking up the work. I modified the code following the comments.
Besides, I tweaked the test code a bit. There are several things I’m not 100% sure. Please see
my replies below.

On Jan 27, 2020, at 11:24 PM, Fujii Masao <masao.fujii@oss.nttdata.com<mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2020/01/15 19:18, Paul Guo wrote:
I further fixed the last test failure (due to a small bug in the test, not in code). Attached are the new patch series. Let's see the CI pipeline result.

Thanks for updating the patches!

I started reading the 0003 patch.

The approach that the 0003 patch uses is not the perfect solution.
If the standby crashes after tblspc_redo() removes the directory and before
its subsequent COMMIT record is replayed, PANIC error would occur since
there can be some unresolved missing directory entries when we reach the
consistent state. The problem would very rarely happen, though...
Just idea; calling XLogFlush() to update the minimum recovery point just
before tblspc_redo() performs destroy_tablespace_directories() may be
safe and helpful for the problem?

Yes looks like an issue. My understanding is the below scenario.

XLogLogMissingDir()

XLogFlush() in redo (e.g. in a commit redo).  <- create a minimum recovery point (we call it LSN_A).

tblspc_redo()->XLogForgetMissingDir()
       <- If we panic immediately after we remove the directory in tblspc_redo()
       <- when we do replay during crash-recovery, we will check consistency at LSN_A and thus PANIC inXLogCheckMissingDirs()

commit

We should add a XLogFlush() in tblspc_redo(). This brings several other questions to my minds also.


1. Should we call XLogFlush() in dbase_redo()  for XLOG_DBASE_DROP also?
   It calls both XLogDropDatabase() and XLogForgetMissingDir, which seem to have this issue also?

2. xact_redo_abort() calls DropRelationFiles() also. Why do not we call XLogFlush() there?



- appendStringInfo(buf, "copy dir %u/%u to %u/%u",
-  xlrec->src_tablespace_id, xlrec->src_db_id,
-  xlrec->tablespace_id, xlrec->db_id);
+ dbpath1 = GetDatabasePath(xlrec->src_db_id,  xlrec->src_tablespace_id);
+ dbpath2 = GetDatabasePath(xlrec->db_id, xlrec->tablespace_id);
+ appendStringInfo(buf, "copy dir %s to %s", dbpath1, dbpath2);
+ pfree(dbpath2);
+ pfree(dbpath1);

If the patch is for the bug fix and would be back-ported, the above change
would lead to change pg_waldump's output for CREATE/DROP DATABASE between
minor versions. IMO it's better to avoid such change and separate the above
as a separate patch only for master.

I know we do not want wal format between minor releases, but does wal description string change
between minor releases affect users? Anyone I’ll extract this part into a separate patch in the series
since this change is actually independent of the other changes..


- appendStringInfo(buf, " %u/%u",
-  xlrec->tablespace_ids[i], xlrec->db_id);
+ {
+ dbpath1 = GetDatabasePath(xlrec->db_id, xlrec->tablespace_ids[i]);
+ appendStringInfo(buf,  "%s", dbpath1);
+ pfree(dbpath1);
+ }

Same as above.

BTW, the above "%s" should be " %s", i.e., a space character needs to be
appended to the head of "%s”.

OK


+ get_parent_directory(parent_path);
+ if (!(stat(parent_path, &st) == 0 && S_ISDIR(st.st_mode)))
+ {
+ XLogLogMissingDir(xlrec->tablespace_id, InvalidOid, dst_path);

The third argument of XLogLogMissingDir() should be parent_path instead of
dst_path?

The argument is for debug message printing so both should be fine, but admittedly we are
logging for the tablespace directory so parent_path might be better.


+ if (hash_search(missing_dir_tab, &key, HASH_REMOVE, NULL) == NULL)
+ elog(DEBUG2, "dir %s tablespace %d database %d is not missing",
+  path, spcNode, dbNode);

I think that this elog() is useless and rather confusing.

OK. Modified.


+ XLogForgetMissingDir(xlrec->ts_id, InvalidOid, "");

The third argument should be set to the actual path instead of an empty
string. Otherwise XLogForgetMissingDir() may emit a confusing DEBUG2
message. Or the third argument of XLogForgetMissingDir() should be removed
and the path in the DEBUG2 message should be calculated from the spcNode
and dbNode in the hash entry in XLogForgetMissingDir().

I’m now removing the third argument. Use GetDatabasePath() to get the path if database did I snot InvalidOid.


+#include "common/file_perm.h"

This seems not necessary.

Right.

Commits

  1. Improve recently-added test reliability

  2. Fix new recovery test for log_error_verbosity=verbose case

  3. Fix test instability

  4. Fix replay of create database records on standby

  5. Allow "in place" tablespaces.

  6. Fix get_dirent_type() for Windows junction points.

  7. Revert "Fix replay of create database records on standby"

  8. Add end-to-end testing of pg_basebackup's tar-format output.

  9. Make DROP DATABASE command generate less WAL records.

  10. Consolidate methods for translating a Perl path to a Windows path.