Re: Attempt to consolidate reading of XLOG page

On Thu, May 2, 2019 at 12:18 PM Antonin Houska <ah@cybertec.at> wrote:
> The next version of the patch is attached.

I don't think any of this looks acceptable:

+#ifndef FRONTEND
+/*
+ * Backend should have wal_segment_size variable initialized, segsize is not
+ * used.
+ */
+#define XLogFileNameCommon(tli, num, segsize) XLogFileNameP((tli), (num))
+#define xlr_error(...) ereport(ERROR, (errcode_for_file_access(),
errmsg(__VA_ARGS__)))
+#else
+static char xlr_error_msg[MAXFNAMELEN];
+#define XLogFileNameCommon(tli, num, segsize)
(XLogFileName(xlr_error_msg, (tli), (num), (segsize)),\
+    xlr_error_msg)
+#include "fe_utils/logging.h"
+/*
+ * Frontend application (currently only pg_waldump.c) cannot catch and further
+ * process errors, so they simply treat them as fatal.
+ */
+#define xlr_error(...) do {pg_log_fatal(__VA_ARGS__);
exit(EXIT_FAILURE); } while(0)
+#endif

The backend part doesn't look OK because depending on the value of a
global variable instead of getting the information via parameters
seems like a step backward.  The frontend part doesn't look OK because
it locks every application that uses the xlogreader stuff into using
pg_log_fatal when an error occurs, which may not be what everybody
wants to do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company