Refactor SLRU download interface between Postgres and the neon extension

Move the responsibility of writing the SLRU segment contents to the
neon extension. The neon extension hook now downloads the file and
places it to the correct path, and returns a true/false to the caller
to indicate if the file existed. On successful download, the caller
retries opening the file.

Author: hlinnaka

src/backend/access/transam/slru.c

@@ -620,23 +620,25 @@ SimpleLruWritePage(SlruCtl ctl, int slotno)
 
 
 /*
- * NEON: we do not want to include large pg_xact/multixact files in basebackup and prefer
- * to download them on demand to reduce startup time.
- * If SLRU segment is not found, we try to download it from page server
+ * NEON: we do not want to include large pg_xact/multixact files in the
+ * basebackup and prefer to download them on demand to reduce startup time.
+ *
+ * If SLRU segment is not found, we try to download it from the pageserver.
+ *
+ * Returns:
+ *   true if the file was successfully downloaded
+ *   false if the file was not found in pageserver
+ *   ereports if some other error happened
  */
-static int
-SimpleLruDownloadSegment(SlruCtl ctl, int pageno, char const* path)
+static bool
+SimpleLruDownloadSegment(SlruCtl ctl, int pageno, char const *path)
 {
-	int segno;
-	int fd = -1;
-	int n_blocks;
-	char* buffer;
-
+	int			segno;
 	static SMgrRelationData dummy_smgr_rel = {0};
 
 	/* If page is greater than latest written page, then do not try to download segment from server */
 	if (ctl->PagePrecedes(ctl->shared->latest_page_number, pageno))
-		return -1;
+		return false;
 
 	if (!dummy_smgr_rel.smgr)
 	{
@@ -645,45 +647,7 @@ SimpleLruDownloadSegment(SlruCtl ctl, int pageno, char const* path)
 	}
 	segno = pageno / SLRU_PAGES_PER_SEGMENT;
 
-	if (neon_use_communicator_worker) {
-		buffer = NULL;
-	} else {
-		buffer = palloc(BLCKSZ * SLRU_PAGES_PER_SEGMENT);
-	}
-
-	n_blocks = smgr_read_slru_segment(&dummy_smgr_rel, path, segno, buffer);
-	if (n_blocks > 0)
-	{
-		fd = OpenTransientFile(path, O_RDWR | O_CREAT | PG_BINARY);
-		if (fd < 0)
-		{
-			slru_errcause = SLRU_OPEN_FAILED;
-			slru_errno = errno;
-			pfree(buffer);
-			return -1;
-		}
-
-		if (!neon_use_communicator_worker) {
-			errno = 0;
-			pgstat_report_wait_start(WAIT_EVENT_SLRU_WRITE);
-			if (pg_pwrite(fd, buffer, n_blocks*BLCKSZ, 0) != n_blocks*BLCKSZ)
-			{
-				pgstat_report_wait_end();
-				/* if write didn't set errno, assume problem is no disk space */
-				if (errno == 0)
-					errno = ENOSPC;
-				slru_errcause = SLRU_WRITE_FAILED;
-				slru_errno = errno;
-
-				CloseTransientFile(fd);
-				pfree(buffer);
-				return -1;
-			}
-			pgstat_report_wait_end();
-		}
-	}
-	pfree(buffer);
-	return fd;
+	return smgr_read_slru_segment(&dummy_smgr_rel, path, segno);
 }
 
 /*
@@ -702,29 +666,34 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
 	int			fd;
 	bool		result;
 	off_t		endpos;
+	bool		attempted_download = false;
 
 	/* update the stats counter of checked pages */
 	pgstat_count_slru_page_exists(ctl->shared->slru_stats_idx);
 
 	SlruFileName(ctl, path, segno);
 
+ retry_after_download:
 	fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
 	if (fd < 0)
 	{
-		/* expected: file doesn't exist */
-		if (errno == ENOENT)
+		if (errno == ENOENT && !attempted_download)
 		{
-			fd = SimpleLruDownloadSegment(ctl, pageno, path);
-			if (fd < 0)
-				return false;
-		}
-		else
-		{
-			/* report error normally */
-			slru_errcause = SLRU_OPEN_FAILED;
-			slru_errno = errno;
-			SlruReportIOError(ctl, pageno, 0);
+			/* Try to download the file from the pageserver */
+			attempted_download = true;
+			if (SimpleLruDownloadSegment(ctl, pageno, path))
+				goto retry_after_download;
+			errno = ENOENT;
 		}
+
+		/* expected: file doesn't exist */
+		if (errno == ENOENT)
+			return false;
+
+		/* report error normally */
+		slru_errcause = SLRU_OPEN_FAILED;
+		slru_errno = errno;
+		SlruReportIOError(ctl, pageno, 0);
 	}
 
 	if ((endpos = lseek(fd, 0, SEEK_END)) < 0)
@@ -765,6 +734,7 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
 	off_t		offset = rpageno * BLCKSZ;
 	char		path[MAXPGPATH];
 	int			fd;
+	bool		attempted_download = false;
 
 	SlruFileName(ctl, path, segno);
 
@@ -775,33 +745,31 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
 	 * SlruPhysicalWritePage).  Hence, if we are InRecovery, allow the case
 	 * where the file doesn't exist, and return zeroes instead.
 	 */
+ retry_after_download:
 	fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
 	if (fd < 0)
 	{
-		if (errno != ENOENT)
+		if (errno == ENOENT && !attempted_download)
+		{
+			/* Try to download the file from the pageserver */
+			attempted_download = true;
+			if (SimpleLruDownloadSegment(ctl, pageno, path))
+				goto retry_after_download;
+			errno = ENOENT;
+		}
+
+		if (errno != ENOENT || !InRecovery)
 		{
 			slru_errcause = SLRU_OPEN_FAILED;
 			slru_errno = errno;
 			return false;
 		}
-		fd = SimpleLruDownloadSegment(ctl, pageno, path);
-		if (fd < 0)
-		{
-			if (!InRecovery)
-			{
-				slru_errcause = SLRU_OPEN_FAILED;
-				slru_errno = errno;
-				return false;
-			}
-			else
-			{
-				ereport(LOG,
-						(errmsg("file \"%s\" doesn't exist, reading as zeroes",
-								path)));
-				MemSet(shared->page_buffer[slotno], 0, BLCKSZ);
-				return true;
-			}
-		}
+
+		ereport(LOG,
+				(errmsg("file \"%s\" doesn't exist, reading as zeroes",
+						path)));
+		MemSet(shared->page_buffer[slotno], 0, BLCKSZ);
+		return true;
 	}
 
 	errno = 0;

src/backend/storage/smgr/smgr.c

@@ -784,10 +784,10 @@ smgr_end_unlogged_build(SMgrRelation reln)
  * of partial segment, it can be smaller. Zero value means that segment doesn't exist.
  * From Postgres point of view empty segment is the same as absent segment.
  */
-int
-smgr_read_slru_segment(SMgrRelation reln, const char* path, int segno, void* buffer)
+bool
+smgr_read_slru_segment(SMgrRelation reln, const char* path, int segno)
 {
-	return (*reln->smgr).smgr_read_slru_segment ? (*reln->smgr).smgr_read_slru_segment(reln, path, segno, buffer) : 0;
+	return (*reln->smgr).smgr_read_slru_segment ? (*reln->smgr).smgr_read_slru_segment(reln, path, segno) : false;
 }
 
 

src/include/miscadmin.h

@@ -170,12 +170,6 @@ extern PGDLLIMPORT bool IsPostmasterEnvironment;
 extern PGDLLIMPORT bool IsUnderPostmaster;
 extern PGDLLIMPORT bool IsBackgroundWorker;
 extern PGDLLIMPORT bool IsBinaryUpgrade;
-/* Whether the communicator worker is used or not. Defined here so that
- * it is also accessible from the main postgres code easily without
- * having to look up the value using strings and chain of other
- * functions.
- */
-extern PGDLLIMPORT bool neon_use_communicator_worker;
 
 extern PGDLLIMPORT bool ExitOnAnyError;
 

src/include/storage/smgr.h

@@ -131,7 +131,7 @@ typedef struct f_smgr
 	void		(*smgr_finish_unlogged_build_phase_1) (SMgrRelation reln);
 	void		(*smgr_end_unlogged_build) (SMgrRelation reln);
 
-	int  		(*smgr_read_slru_segment) (SMgrRelation reln, const char *path, int segno, void* buffer);
+	bool  		(*smgr_read_slru_segment) (SMgrRelation reln, const char *path, int segno);
 } f_smgr;
 
 typedef void (*smgr_init_hook_type) (void);
@@ -192,6 +192,6 @@ extern void smgr_start_unlogged_build(SMgrRelation reln);
 extern void	smgr_finish_unlogged_build_phase_1(SMgrRelation reln);
 extern void smgr_end_unlogged_build(SMgrRelation reln);
 
-extern int  smgr_read_slru_segment(SMgrRelation reln, const char *path, int segno, void* buffer);
+extern bool smgr_read_slru_segment(SMgrRelation reln, const char *path, int segno);
 
 #endif							/* SMGR_H */