public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v18 3/4] Move removal of old logical rewrite mapping files to custodian.
41+ messages / 7 participants
[nested] [flat]
* [PATCH v18 3/4] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
contrib/test_decoding/expected/rewrite.out | 19 ++++++
contrib/test_decoding/sql/rewrite.sql | 14 ++++
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++---
src/backend/postmaster/custodian.c | 43 ++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
6 files changed, 149 insertions(+), 10 deletions(-)
diff --git a/contrib/test_decoding/expected/rewrite.out b/contrib/test_decoding/expected/rewrite.out
index 8b97f15f6f..214a514a0a 100644
--- a/contrib/test_decoding/expected/rewrite.out
+++ b/contrib/test_decoding/expected/rewrite.out
@@ -183,3 +183,22 @@ SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
t
(1 row)
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
+ ?column?
+----------
+ t
+(1 row)
+
diff --git a/contrib/test_decoding/sql/rewrite.sql b/contrib/test_decoding/sql/rewrite.sql
index d268fa559a..d66f70f837 100644
--- a/contrib/test_decoding/sql/rewrite.sql
+++ b/contrib/test_decoding/sql/rewrite.sql
@@ -122,3 +122,17 @@ BEGIN
END
$$;
SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2fe9e48e50..ff4cd8cef9 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,9 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS, LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1245,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1283,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 9382d524a6..33185e9913 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(void);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -72,6 +77,7 @@ struct cust_task_funcs_entry
*/
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -377,3 +383,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index ab6d4283b9..00280c203b 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -19,6 +21,7 @@
typedef enum CustodianTask
{
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -28,5 +31,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--LZvS9be/3tNcYl/X
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v18-0004-Do-not-delay-shutdown-due-to-long-running-custod.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v11 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2f08fbe8d3..a01edf8a1f 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -117,6 +117,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -124,6 +125,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1183,7 +1185,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1211,6 +1214,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1243,15 +1251,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1289,3 +1289,61 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--envbJBWh7q8WU6mo--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v10 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2f08fbe8d3..a01edf8a1f 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -117,6 +117,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -124,6 +125,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1183,7 +1185,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1211,6 +1214,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1243,15 +1251,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1289,3 +1289,61 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 353cbb2924..965372b5ff 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--+QahgC5+KEYLbs62--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v5 6/8] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
---
src/backend/access/heap/rewriteheap.c | 83 +++++++++++++++++++++++----
src/backend/postmaster/checkpointer.c | 33 +++++++++++
src/backend/postmaster/custodian.c | 10 ++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/bgwriter.h | 3 +
5 files changed, 120 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..c5a1103687 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,10 +116,13 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/bgwriter.h"
+#include "postmaster/interrupt.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
#include "storage/fd.h"
+#include "storage/proc.h"
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
@@ -1182,7 +1185,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1210,6 +1214,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ CheckPointSetLogicalRewriteCutoff(cutoff);
+ if (ProcGlobal->custodianLatch)
+ SetLatch(ProcGlobal->custodianLatch);
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1240,15 +1249,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1286,3 +1287,65 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't interfere with an
+ * ongoing call to CheckPointLogicalRewriteHeap() that is flushing mappings to
+ * disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+ bool value_set = false;
+
+ cutoff = CheckPointGetLogicalRewriteCutoff(&value_set);
+ if (!value_set)
+ return;
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while (!ShutdownRequestPending &&
+ (mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 4488e3a443..666f2a0368 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -128,6 +128,9 @@ typedef struct
uint32 num_backend_writes; /* counts user backend buffer writes */
uint32 num_backend_fsync; /* counts user backend fsync calls */
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
+ bool logical_rewrite_mappings_cutoff_set;
+
int num_requests; /* current # of requests */
int max_requests; /* allocated array size */
CheckpointerRequest requests[FLEXIBLE_ARRAY_MEMBER];
@@ -1342,3 +1345,33 @@ FirstCallSinceLastCheckpoint(void)
return FirstCall;
}
+
+/*
+ * Used by CheckPointLogicalRewriteHeap() to tell the custodian which logical
+ * rewrite mapping files it can remove.
+ */
+void
+CheckPointSetLogicalRewriteCutoff(XLogRecPtr cutoff)
+{
+ SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
+ CheckpointerShmem->logical_rewrite_mappings_cutoff = cutoff;
+ CheckpointerShmem->logical_rewrite_mappings_cutoff_set = true;
+ SpinLockRelease(&CheckpointerShmem->ckpt_lck);
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CheckPointGetLogicalRewriteCutoff(bool *value_set)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
+ cutoff = CheckpointerShmem->logical_rewrite_mappings_cutoff;
+ *value_set = CheckpointerShmem->logical_rewrite_mappings_cutoff_set;
+ SpinLockRelease(&CheckpointerShmem->ckpt_lck);
+
+ return cutoff;
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 8591c5db9b..7f914a617f 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -36,6 +36,7 @@
#include <time.h>
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -219,6 +220,15 @@ CustodianMain(void)
*/
RemoveOldSerializedSnapshots();
+ /*
+ * Remove logical rewrite mapping files that are no longer needed.
+ *
+ * It is not important for these to be removed in single-user mode, so
+ * we don't need any extra handling outside of the custodian process for
+ * this.
+ */
+ RemoveOldLogicalRewriteMappings();
+
/* Calculate how long to sleep */
end_time = (pg_time_t) time(NULL);
elapsed_secs = end_time - start_time;
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index aa5c48f219..f493094557 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
void CheckPointLogicalRewriteHeap(void);
+void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/bgwriter.h b/src/include/postmaster/bgwriter.h
index 2882efd67b..051e6732cb 100644
--- a/src/include/postmaster/bgwriter.h
+++ b/src/include/postmaster/bgwriter.h
@@ -42,4 +42,7 @@ extern void CheckpointerShmemInit(void);
extern bool FirstCallSinceLastCheckpoint(void);
+extern void CheckPointSetLogicalRewriteCutoff(XLogRecPtr cutoff);
+extern XLogRecPtr CheckPointGetLogicalRewriteCutoff(bool *value_set);
+
#endif /* _BGWRITER_H */
--
2.25.1
--2oS5YaxWCcQjTEyO
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5-0007-Use-syncfs-in-CheckPointLogicalRewriteHeap-for-sh.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v4 6/8] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
---
src/backend/access/heap/rewriteheap.c | 83 +++++++++++++++++++++++----
src/backend/postmaster/checkpointer.c | 33 +++++++++++
src/backend/postmaster/custodian.c | 10 ++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/bgwriter.h | 3 +
5 files changed, 120 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..c5a1103687 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,10 +116,13 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/bgwriter.h"
+#include "postmaster/interrupt.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
#include "storage/fd.h"
+#include "storage/proc.h"
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
@@ -1182,7 +1185,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1210,6 +1214,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ CheckPointSetLogicalRewriteCutoff(cutoff);
+ if (ProcGlobal->custodianLatch)
+ SetLatch(ProcGlobal->custodianLatch);
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1240,15 +1249,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1286,3 +1287,65 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't interfere with an
+ * ongoing call to CheckPointLogicalRewriteHeap() that is flushing mappings to
+ * disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+ bool value_set = false;
+
+ cutoff = CheckPointGetLogicalRewriteCutoff(&value_set);
+ if (!value_set)
+ return;
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while (!ShutdownRequestPending &&
+ (mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 23f691cd47..fe0934e5a6 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -127,6 +127,9 @@ typedef struct
uint32 num_backend_writes; /* counts user backend buffer writes */
uint32 num_backend_fsync; /* counts user backend fsync calls */
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
+ bool logical_rewrite_mappings_cutoff_set;
+
int num_requests; /* current # of requests */
int max_requests; /* allocated array size */
CheckpointerRequest requests[FLEXIBLE_ARRAY_MEMBER];
@@ -1341,3 +1344,33 @@ FirstCallSinceLastCheckpoint(void)
return FirstCall;
}
+
+/*
+ * Used by CheckPointLogicalRewriteHeap() to tell the custodian which logical
+ * rewrite mapping files it can remove.
+ */
+void
+CheckPointSetLogicalRewriteCutoff(XLogRecPtr cutoff)
+{
+ SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
+ CheckpointerShmem->logical_rewrite_mappings_cutoff = cutoff;
+ CheckpointerShmem->logical_rewrite_mappings_cutoff_set = true;
+ SpinLockRelease(&CheckpointerShmem->ckpt_lck);
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CheckPointGetLogicalRewriteCutoff(bool *value_set)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
+ cutoff = CheckpointerShmem->logical_rewrite_mappings_cutoff;
+ *value_set = CheckpointerShmem->logical_rewrite_mappings_cutoff_set;
+ SpinLockRelease(&CheckpointerShmem->ckpt_lck);
+
+ return cutoff;
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 0f4dbdd669..9c5479b5cf 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -36,6 +36,7 @@
#include <time.h>
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -218,6 +219,15 @@ CustodianMain(void)
*/
RemoveOldSerializedSnapshots();
+ /*
+ * Remove logical rewrite mapping files that are no longer needed.
+ *
+ * It is not important for these to be removed in single-user mode, so
+ * we don't need any extra handling outside of the custodian process for
+ * this.
+ */
+ RemoveOldLogicalRewriteMappings();
+
/* Calculate how long to sleep */
end_time = (pg_time_t) time(NULL);
elapsed_secs = end_time - start_time;
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index aa5c48f219..f493094557 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
void CheckPointLogicalRewriteHeap(void);
+void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/bgwriter.h b/src/include/postmaster/bgwriter.h
index 2882efd67b..051e6732cb 100644
--- a/src/include/postmaster/bgwriter.h
+++ b/src/include/postmaster/bgwriter.h
@@ -42,4 +42,7 @@ extern void CheckpointerShmemInit(void);
extern bool FirstCallSinceLastCheckpoint(void);
+extern void CheckPointSetLogicalRewriteCutoff(XLogRecPtr cutoff);
+extern XLogRecPtr CheckPointGetLogicalRewriteCutoff(bool *value_set);
+
#endif /* _BGWRITER_H */
--
2.25.1
--BXVAT5kNtrzKuDFl
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0007-Use-syncfs-in-CheckPointLogicalRewriteHeap-for-sh.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v17 3/4] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
contrib/test_decoding/expected/rewrite.out | 19 ++++++
contrib/test_decoding/sql/rewrite.sql | 14 ++++
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++---
src/backend/postmaster/custodian.c | 43 ++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
6 files changed, 149 insertions(+), 10 deletions(-)
diff --git a/contrib/test_decoding/expected/rewrite.out b/contrib/test_decoding/expected/rewrite.out
index 8b97f15f6f..214a514a0a 100644
--- a/contrib/test_decoding/expected/rewrite.out
+++ b/contrib/test_decoding/expected/rewrite.out
@@ -183,3 +183,22 @@ SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
t
(1 row)
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
+ ?column?
+----------
+ t
+(1 row)
+
diff --git a/contrib/test_decoding/sql/rewrite.sql b/contrib/test_decoding/sql/rewrite.sql
index d268fa559a..d66f70f837 100644
--- a/contrib/test_decoding/sql/rewrite.sql
+++ b/contrib/test_decoding/sql/rewrite.sql
@@ -122,3 +122,17 @@ BEGIN
END
$$;
SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2fe9e48e50..ff4cd8cef9 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,9 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS, LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1245,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1283,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index d0fd955d4b..c4d0a22451 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(void);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -72,6 +77,7 @@ struct cust_task_funcs_entry
*/
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -382,3 +388,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index ab6d4283b9..00280c203b 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -19,6 +21,7 @@
typedef enum CustodianTask
{
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -28,5 +31,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--ew6BAiZeqk4r7MaW
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v17-0004-Do-not-delay-shutdown-due-to-long-running-custod.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v16 3/4] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
contrib/test_decoding/expected/rewrite.out | 21 ++++++
contrib/test_decoding/sql/rewrite.sql | 17 +++++
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++---
src/backend/postmaster/custodian.c | 43 ++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
6 files changed, 154 insertions(+), 10 deletions(-)
diff --git a/contrib/test_decoding/expected/rewrite.out b/contrib/test_decoding/expected/rewrite.out
index b30999c436..00b505ef67 100644
--- a/contrib/test_decoding/expected/rewrite.out
+++ b/contrib/test_decoding/expected/rewrite.out
@@ -152,6 +152,27 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
COMMIT
(6 rows)
+-- make sure custodian cleans up files
+CHECKPOINT;
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
+ ?column?
+----------
+ t
+(1 row)
+
SELECT pg_drop_replication_slot('regression_slot');
pg_drop_replication_slot
--------------------------
diff --git a/contrib/test_decoding/sql/rewrite.sql b/contrib/test_decoding/sql/rewrite.sql
index 62dead3a9b..767eccbed4 100644
--- a/contrib/test_decoding/sql/rewrite.sql
+++ b/contrib/test_decoding/sql/rewrite.sql
@@ -100,6 +100,23 @@ VACUUM FULL pg_proc; VACUUM FULL pg_description; VACUUM FULL pg_shdescription; V
INSERT INTO replication_example(somedata, testcolumn1, testcolumn3) VALUES (9, 7, 1);
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+-- make sure custodian cleans up files
+CHECKPOINT;
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
+
SELECT pg_drop_replication_slot('regression_slot');
DROP TABLE IF EXISTS replication_example;
DROP FUNCTION iamalongfunction();
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2fe9e48e50..ff4cd8cef9 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,9 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS, LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1245,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1283,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index d0fd955d4b..c4d0a22451 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(void);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -72,6 +77,7 @@ struct cust_task_funcs_entry
*/
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -382,3 +388,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index ab6d4283b9..00280c203b 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -19,6 +21,7 @@
typedef enum CustodianTask
{
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -28,5 +31,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--lrZ03NoBR/3+SXJZ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v16-0004-Do-not-delay-shutdown-due-to-long-running-custod.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v15 3/4] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2fe9e48e50..ff4cd8cef9 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,9 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS, LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1245,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1283,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index d0fd955d4b..c4d0a22451 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(void);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -72,6 +77,7 @@ struct cust_task_funcs_entry
*/
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -382,3 +388,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index ab6d4283b9..00280c203b 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -19,6 +21,7 @@
typedef enum CustodianTask
{
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -28,5 +31,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--HcAYCG3uE/tztfnV
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v15-0004-Do-not-delay-shutdown-due-to-long-running-custod.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v14 3/3] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2fe9e48e50..ff4cd8cef9 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,9 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS, LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1245,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1283,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index d0fd955d4b..c4d0a22451 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(void);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -72,6 +77,7 @@ struct cust_task_funcs_entry
*/
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -382,3 +388,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index ab6d4283b9..00280c203b 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -19,6 +21,7 @@
typedef enum CustodianTask
{
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -28,5 +31,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--BXVAT5kNtrzKuDFl--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v6 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
---
src/backend/access/heap/rewriteheap.c | 79 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 44 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 5 ++
4 files changed, 119 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..edeab65e60 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -1182,7 +1183,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1210,6 +1212,10 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ CustodianSetLogicalRewriteCutoff(cutoff);
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS);
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1240,15 +1246,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1286,3 +1284,64 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't interfere with an
+ * ongoing call to CheckPointLogicalRewriteHeap() that is flushing mappings to
+ * disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+ bool value_set = false;
+
+ cutoff = CustodianGetLogicalRewriteCutoff(&value_set);
+ if (!value_set)
+ return;
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 861de882c6..0ce4edcf61 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -27,6 +27,7 @@
#include <time.h>
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -46,6 +47,9 @@ typedef struct
{
slock_t cust_lck;
int cust_flags;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
+ bool logical_rewrite_mappings_cutoff_set;
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -222,6 +226,16 @@ CustodianMain(void)
if (flags & CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS)
RemoveOldSerializedSnapshots();
+ /*
+ * Remove logical rewrite mapping files that are no longer needed.
+ *
+ * It is not important for these to be removed in single-user mode, so
+ * we don't need any extra handling outside of the custodian process for
+ * this.
+ */
+ if (flags & CUSTODIAN_REMOVE_REWRITE_MAPPINGS)
+ RemoveOldLogicalRewriteMappings();
+
/* Calculate how long to sleep */
end_time = (pg_time_t) time(NULL);
elapsed_secs = end_time - start_time;
@@ -274,3 +288,33 @@ RequestCustodian(int flags)
if (ProcGlobal->custodianLatch)
SetLatch(ProcGlobal->custodianLatch);
}
+
+/*
+ * Used by CheckPointLogicalRewriteHeap() to tell the custodian which logical
+ * rewrite mapping files it can remove.
+ */
+void
+CustodianSetLogicalRewriteCutoff(XLogRecPtr cutoff)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = cutoff;
+ CustodianShmem->logical_rewrite_mappings_cutoff_set = true;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(bool *value_set)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ *value_set = CustodianShmem->logical_rewrite_mappings_cutoff_set;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 3e27790b3f..61d7aa8ed8 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 769c07f2c9..1af96ebd02 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,13 +12,18 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(int flags);
+extern void CustodianSetLogicalRewriteCutoff(XLogRecPtr cutoff);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(bool *value_set);
/* flags for RequestCustodian() */
#define CUSTODIAN_REMOVE_TEMP_FILES 0x0001
#define CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS 0x0002
+#define CUSTODIAN_REMOVE_REWRITE_MAPPINGS 0x0004
#endif /* _CUSTODIAN_H */
--
2.25.1
--liOOAslEiF7prFVr--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v10 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2f08fbe8d3..a01edf8a1f 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -117,6 +117,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -124,6 +125,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1183,7 +1185,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1211,6 +1214,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1243,15 +1251,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1289,3 +1289,61 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 353cbb2924..965372b5ff 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--+QahgC5+KEYLbs62--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v20 3/4] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
contrib/test_decoding/expected/rewrite.out | 19 ++++++
contrib/test_decoding/sql/rewrite.sql | 14 ++++
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++---
src/backend/postmaster/custodian.c | 43 ++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
6 files changed, 149 insertions(+), 10 deletions(-)
diff --git a/contrib/test_decoding/expected/rewrite.out b/contrib/test_decoding/expected/rewrite.out
index 8b97f15f6f..214a514a0a 100644
--- a/contrib/test_decoding/expected/rewrite.out
+++ b/contrib/test_decoding/expected/rewrite.out
@@ -183,3 +183,22 @@ SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
t
(1 row)
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
+ ?column?
+----------
+ t
+(1 row)
+
diff --git a/contrib/test_decoding/sql/rewrite.sql b/contrib/test_decoding/sql/rewrite.sql
index d268fa559a..d66f70f837 100644
--- a/contrib/test_decoding/sql/rewrite.sql
+++ b/contrib/test_decoding/sql/rewrite.sql
@@ -122,3 +122,17 @@ BEGIN
END
$$;
SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 8993c1ed5a..9ea0f81ac3 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,9 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS, LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1245,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1283,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 4e0ce1f7b3..4cbd89fae9 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(void);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -72,6 +77,7 @@ struct cust_task_funcs_entry
*/
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -377,3 +383,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 1125457053..dc3eb3e308 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index ab6d4283b9..00280c203b 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -19,6 +21,7 @@
typedef enum CustodianTask
{
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -28,5 +31,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--CE+1k2dSO48ffgeK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v20-0004-Do-not-delay-shutdown-due-to-long-running-custod.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v19 3/4] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
contrib/test_decoding/expected/rewrite.out | 19 ++++++
contrib/test_decoding/sql/rewrite.sql | 14 ++++
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++---
src/backend/postmaster/custodian.c | 43 ++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
6 files changed, 149 insertions(+), 10 deletions(-)
diff --git a/contrib/test_decoding/expected/rewrite.out b/contrib/test_decoding/expected/rewrite.out
index 8b97f15f6f..214a514a0a 100644
--- a/contrib/test_decoding/expected/rewrite.out
+++ b/contrib/test_decoding/expected/rewrite.out
@@ -183,3 +183,22 @@ SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
t
(1 row)
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
+ ?column?
+----------
+ t
+(1 row)
+
diff --git a/contrib/test_decoding/sql/rewrite.sql b/contrib/test_decoding/sql/rewrite.sql
index d268fa559a..d66f70f837 100644
--- a/contrib/test_decoding/sql/rewrite.sql
+++ b/contrib/test_decoding/sql/rewrite.sql
@@ -122,3 +122,17 @@ BEGIN
END
$$;
SELECT count(*) = 0 FROM pg_ls_logicalsnapdir();
+DO $$
+DECLARE
+ mappings_removed bool;
+ loops int := 0;
+BEGIN
+ LOOP
+ mappings_removed := count(*) = 0 FROM pg_ls_logicalmapdir();
+ IF mappings_removed OR loops > 120 * 100 THEN EXIT; END IF;
+ PERFORM pg_sleep(0.01);
+ loops := loops + 1;
+ END LOOP;
+END
+$$;
+SELECT count(*) = 0 FROM pg_ls_logicalmapdir();
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 8993c1ed5a..9ea0f81ac3 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,9 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS, LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1245,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1283,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 4e0ce1f7b3..4cbd89fae9 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(void);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -72,6 +77,7 @@ struct cust_task_funcs_entry
*/
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -377,3 +383,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 1125457053..dc3eb3e308 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index ab6d4283b9..00280c203b 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -19,6 +21,7 @@
typedef enum CustodianTask
{
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -28,5 +31,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--x+6KMIRAuhnl3hBn
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v19-0004-Do-not-delay-shutdown-due-to-long-running-custod.patch"
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v13 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 80 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 ++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 118 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2fe9e48e50..07976504cc 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1247,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1285,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--CE+1k2dSO48ffgeK--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v12 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 80 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 ++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 118 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2fe9e48e50..07976504cc 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1179,7 +1181,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1207,6 +1210,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1239,15 +1247,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1285,3 +1285,63 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+ PGFileType de_type;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ de_type = get_dirent_type(path, mapping_de, false, DEBUG1);
+
+ if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 5cc04756a5..bc875330d7 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--UlVJffcvxoiEqYs2--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v9 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 9dd885d936..a08dd4a524 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1182,7 +1184,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1210,6 +1213,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1240,15 +1248,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1286,3 +1286,61 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 353cbb2924..965372b5ff 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--y0ulUmNC+osPPQO6--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v8 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 9dd885d936..a08dd4a524 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1182,7 +1184,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1210,6 +1213,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1240,15 +1248,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1286,3 +1286,61 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 353cbb2924..965372b5ff 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--X1bOJ3K7DJ5YkBrT--
^ permalink raw reply [nested|flat] 41+ messages in thread
* [PATCH v7 6/6] Move removal of old logical rewrite mapping files to custodian.
@ 2021-12-13 06:07 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Nathan Bossart @ 2021-12-13 06:07 UTC (permalink / raw)
If there are many such files to remove, checkpoints can take much
longer. To avoid this, move this work to the newly-introduced
custodian process.
Since the mapping files include 32-bit transaction IDs, there is a
risk of wraparound if the files are not cleaned up fast enough.
Removing these files in checkpoints offered decent wraparound
protection simply due to the relatively high frequency of
checkpointing. With this change, servers should still clean up
mappings files with decently high frequency, but in theory the
wraparound risk might worsen for some (e.g., if the custodian is
spending a lot of time on a different task). Given this is an
existing problem, this change makes no effort to handle the
wraparound risk, and it is left as a future exercise.
---
src/backend/access/heap/rewriteheap.c | 78 +++++++++++++++++++++++----
src/backend/postmaster/custodian.c | 43 +++++++++++++++
src/include/access/rewriteheap.h | 1 +
src/include/postmaster/custodian.h | 4 ++
4 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 197f06b5ec..6de1fb19de 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -116,6 +116,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "postmaster/custodian.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "storage/bufmgr.h"
@@ -123,6 +124,7 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
#include "utils/rel.h"
/*
@@ -1182,7 +1184,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
* Perform a checkpoint for logical rewrite mappings
*
* This serves two tasks:
- * 1) Remove all mappings not needed anymore based on the logical restart LSN
+ * 1) Alert the custodian to remove all mappings not needed anymore based on the
+ * logical restart LSN
* 2) Flush all remaining mappings to disk, so that replay after a checkpoint
* only has to deal with the parts of a mapping that have been written out
* after the checkpoint started.
@@ -1210,6 +1213,11 @@ CheckPointLogicalRewriteHeap(void)
if (cutoff != InvalidXLogRecPtr && redo < cutoff)
cutoff = redo;
+ /* let the custodian know what it can remove */
+ RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
+ !IsUnderPostmaster,
+ LSNGetDatum(cutoff));
+
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
@@ -1240,15 +1248,7 @@ CheckPointLogicalRewriteHeap(void)
lsn = ((uint64) hi) << 32 | lo;
- if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
- {
- elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
- if (unlink(path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- else
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
{
/* on some operating systems fsyncing a file requires O_RDWR */
int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
@@ -1286,3 +1286,61 @@ CheckPointLogicalRewriteHeap(void)
/* persist directory entries to disk */
fsync_fname("pg_logical/mappings", true);
}
+
+/*
+ * Remove all mappings not needed anymore based on the logical restart LSN saved
+ * by the checkpointer. We use this saved value instead of calling
+ * ReplicationSlotsComputeLogicalRestartLSN() so that we don't try to remove
+ * files that a concurrent call to CheckPointLogicalRewriteHeap() is trying to
+ * flush to disk.
+ */
+void
+RemoveOldLogicalRewriteMappings(void)
+{
+ XLogRecPtr cutoff;
+ DIR *mappings_dir;
+ struct dirent *mapping_de;
+ char path[MAXPGPATH + 20];
+
+ cutoff = CustodianGetLogicalRewriteCutoff();
+
+ mappings_dir = AllocateDir("pg_logical/mappings");
+ while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ {
+ struct stat statbuf;
+ Oid dboid;
+ Oid relid;
+ XLogRecPtr lsn;
+ TransactionId rewrite_xid;
+ TransactionId create_xid;
+ uint32 hi,
+ lo;
+
+ if (strcmp(mapping_de->d_name, ".") == 0 ||
+ strcmp(mapping_de->d_name, "..") == 0)
+ continue;
+
+ snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
+ if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ continue;
+
+ /* Skip over files that cannot be ours. */
+ if (strncmp(mapping_de->d_name, "map-", 4) != 0)
+ continue;
+
+ if (sscanf(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
+ &dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
+ elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+
+ lsn = ((uint64) hi) << 32 | lo;
+ if (lsn >= cutoff && cutoff != InvalidXLogRecPtr)
+ continue;
+
+ elog(DEBUG1, "removing logical rewrite file \"%s\"", path);
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ FreeDir(mappings_dir);
+}
diff --git a/src/backend/postmaster/custodian.c b/src/backend/postmaster/custodian.c
index 855a756ca0..d4be19e5de 100644
--- a/src/backend/postmaster/custodian.c
+++ b/src/backend/postmaster/custodian.c
@@ -21,6 +21,7 @@
*/
#include "postgres.h"
+#include "access/rewriteheap.h"
#include "libpq/pqsignal.h"
#include "pgstat.h"
#include "postmaster/custodian.h"
@@ -33,11 +34,13 @@
#include "storage/procsignal.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
+#include "utils/pg_lsn.h"
static void DoCustodianTasks(bool retry);
static CustodianTask CustodianGetNextTask(void);
static void CustodianEnqueueTask(CustodianTask task);
static const struct cust_task_funcs_entry *LookupCustodianFunctions(CustodianTask task);
+static void CustodianSetLogicalRewriteCutoff(Datum arg);
typedef struct
{
@@ -45,6 +48,8 @@ typedef struct
CustodianTask task_queue_elems[NUM_CUSTODIAN_TASKS];
int task_queue_head;
+
+ XLogRecPtr logical_rewrite_mappings_cutoff; /* can remove older mappings */
} CustodianShmemStruct;
static CustodianShmemStruct *CustodianShmem;
@@ -73,6 +78,7 @@ struct cust_task_funcs_entry
static const struct cust_task_funcs_entry cust_task_functions[] = {
{CUSTODIAN_REMOVE_TEMP_FILES, RemovePgTempFiles, NULL},
{CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS, RemoveOldSerializedSnapshots, NULL},
+ {CUSTODIAN_REMOVE_REWRITE_MAPPINGS, RemoveOldLogicalRewriteMappings, CustodianSetLogicalRewriteCutoff},
{INVALID_CUSTODIAN_TASK, NULL, NULL} /* must be last */
};
@@ -384,3 +390,40 @@ LookupCustodianFunctions(CustodianTask task)
elog(ERROR, "could not lookup functions for custodian task %d", task);
pg_unreachable();
}
+
+/*
+ * Stores the provided cutoff LSN in the custodian's shared memory.
+ *
+ * It's okay if the cutoff LSN is updated before a previously set cutoff has
+ * been used for cleaning up files. If that happens, it just means that the
+ * next invocation of RemoveOldLogicalRewriteMappings() will use a more accurate
+ * cutoff.
+ */
+static void
+CustodianSetLogicalRewriteCutoff(Datum arg)
+{
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ CustodianShmem->logical_rewrite_mappings_cutoff = DatumGetLSN(arg);
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ /* if pass-by-ref, free Datum memory */
+#ifndef USE_FLOAT8_BYVAL
+ pfree(DatumGetPointer(arg));
+#endif
+}
+
+/*
+ * Used by the custodian to determine which logical rewrite mapping files it can
+ * remove.
+ */
+XLogRecPtr
+CustodianGetLogicalRewriteCutoff(void)
+{
+ XLogRecPtr cutoff;
+
+ SpinLockAcquire(&CustodianShmem->cust_lck);
+ cutoff = CustodianShmem->logical_rewrite_mappings_cutoff;
+ SpinLockRelease(&CustodianShmem->cust_lck);
+
+ return cutoff;
+}
diff --git a/src/include/access/rewriteheap.h b/src/include/access/rewriteheap.h
index 353cbb2924..965372b5ff 100644
--- a/src/include/access/rewriteheap.h
+++ b/src/include/access/rewriteheap.h
@@ -53,5 +53,6 @@ typedef struct LogicalRewriteMappingData
*/
#define LOGICAL_REWRITE_FORMAT "map-%x-%x-%X_%X-%x-%x"
extern void CheckPointLogicalRewriteHeap(void);
+extern void RemoveOldLogicalRewriteMappings(void);
#endif /* REWRITE_HEAP_H */
diff --git a/src/include/postmaster/custodian.h b/src/include/postmaster/custodian.h
index 37334941cc..f177d55159 100644
--- a/src/include/postmaster/custodian.h
+++ b/src/include/postmaster/custodian.h
@@ -12,6 +12,8 @@
#ifndef _CUSTODIAN_H
#define _CUSTODIAN_H
+#include "access/xlogdefs.h"
+
/*
* If you add a new task here, be sure to add its corresponding function
* pointers to cust_task_functions in custodian.c.
@@ -20,6 +22,7 @@ typedef enum CustodianTask
{
CUSTODIAN_REMOVE_TEMP_FILES,
CUSTODIAN_REMOVE_SERIALIZED_SNAPSHOTS,
+ CUSTODIAN_REMOVE_REWRITE_MAPPINGS,
NUM_CUSTODIAN_TASKS, /* new tasks go above */
INVALID_CUSTODIAN_TASK
@@ -29,5 +32,6 @@ extern void CustodianMain(void) pg_attribute_noreturn();
extern Size CustodianShmemSize(void);
extern void CustodianShmemInit(void);
extern void RequestCustodian(CustodianTask task, bool immediate, Datum arg);
+extern XLogRecPtr CustodianGetLogicalRewriteCutoff(void);
#endif /* _CUSTODIAN_H */
--
2.25.1
--/04w6evG8XlLl3ft--
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-07 20:03 Mark Dilger <[email protected]>
0 siblings, 2 replies; 41+ messages in thread
From: Mark Dilger @ 2022-03-07 20:03 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
> On Mar 7, 2022, at 12:01 PM, Robert Haas <[email protected]> wrote:
>
> It's been pointed out upthread that this would have undesirable
> security implications, because the admin option would be inherited,
> and the implicit permission isn't.
Right, but with a reflexive self-admin-option, we could document that it works in a non-inherited way. We'd just be saying the current hard-coded behavior is an option which can be revoked rather than something you're stuck with.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-07 20:09 Mark Dilger <[email protected]>
parent: Mark Dilger <[email protected]>
1 sibling, 0 replies; 41+ messages in thread
From: Mark Dilger @ 2022-03-07 20:09 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
> On Mar 7, 2022, at 12:03 PM, Mark Dilger <[email protected]> wrote:
>
> Right, but with a reflexive self-admin-option, we could document that it works in a non-inherited way. We'd just be saying the current hard-coded behavior is an option which can be revoked rather than something you're stuck with.
We could also say that the default is to not have admin option on yourself, with that being something grantable, but that is a larger change from the historical behavior and might have more consequences for dump/restore, etc.
My concern about just nuking self-admin is that there may be sites which use self-admin and we'd be leaving them without a simple work-around after upgrade, because they couldn't restore the behavior by executing a grant. They'd have to more fundamentally restructure their role relationships to not depend on self-admin, something which might be harder for them to do. Perhaps nobody is using self-admin, or very few people are using it, and I'm being overly concerned.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-07 20:16 Tom Lane <[email protected]>
parent: Mark Dilger <[email protected]>
1 sibling, 2 replies; 41+ messages in thread
From: Tom Lane @ 2022-03-07 20:16 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Robert Haas <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Mark Dilger <[email protected]> writes:
>> On Mar 7, 2022, at 12:01 PM, Robert Haas <[email protected]> wrote:
>>
>> It's been pointed out upthread that this would have undesirable
>> security implications, because the admin option would be inherited,
>> and the implicit permission isn't.
> Right, but with a reflexive self-admin-option, we could document that it works in a non-inherited way. We'd just be saying the current hard-coded behavior is an option which can be revoked rather than something you're stuck with.
After reflection, I think that role self-admin is probably a bad idea that
we should stay away from. It could perhaps be reasonable given some other
system design and/or syntax than what SQL gives us, but we're dealing in
SQL. It doesn't make sense to GRANT a role to itself, and therefore it
likewise doesn't make sense to GRANT WITH ADMIN OPTION.
Based on Robert's archaeological dig, it now seems that the fact that
we have any such behavior at all was just a mistake. What would be
lost if we drop it?
Having said that, one thing that I find fishy is that it's not clear
where the admin privilege for a role originates. After "CREATE ROLE
alice", alice has no members, therefore none that have admin privilege,
therefore the only way that the first member could be added is via
superuser deus ex machina. This does not seem clean. If we recorded
which user created the role, we could act as though that user has
admin privilege (whether or not it's a member). Perhaps I'm
reinventing something that was already discussed upthread. I wonder
what the SQL spec has to say on this point, too.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-07 21:34 David G. Johnston <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 0 replies; 41+ messages in thread
From: David G. Johnston @ 2022-03-07 21:34 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Mark Dilger <[email protected]>; Robert Haas <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Mon, Mar 7, 2022 at 1:16 PM Tom Lane <[email protected]> wrote:
> Based on Robert's archaeological dig, it now seems that the fact that
> we have any such behavior at all was just a mistake. What would be
> lost if we drop it?
>
Probably nothing that couldn't be replaced, and with a better model, but I
do have a concern that there are setups in the wild inadvertently using
this behavior. Enough so that I would vote to change it but include a
migration GUC to restore the current behavior, probably with a deprecation
warning. Kinda depends on the post-change dump/restore mechanics. But
just tearing it out wouldn't seem extraordinary for us.
>
> Having said that, one thing that I find fishy is that it's not clear
> where the admin privilege for a role originates.
I do not see a problem with there being no inherent admin privilege for a
role. A superuser or CREATEROLE user holds admin privilege on all roles in
the cluster. They can delegate the privilege to administer a role to yet
another role in the system. The necessitates creating two roles - the one
being administered and the one being delegated to. I don't see a benefit
to saving which specific superuser or CREATEROLE user "owns" the role that
is to be administered. Not unless non-owner CREATEROLE users are prevented
from exercising admin privileges on the role. That all said, I'd accept
the choice to include such ownership information as a requirement for
meeting the auditing needs of DBAs. But I would argue that such auditing
probably needs to be external to the working system - the fact that
ownership can be changed reduces the benefit of an in-database value.
> If we recorded
which user created the role, we could act as though that user has
> admin privilege (whether or not it's a member).
I suppose we could record the current owner of a role but that seems
unnecessary. I dislike using the "created" concept by virtue of the fact
that, for routines, "security definer" implies creator but it actually
means "security owner".
David J.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-08 04:14 Mark Dilger <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 41+ messages in thread
From: Mark Dilger @ 2022-03-08 04:14 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
> On Mar 7, 2022, at 12:16 PM, Tom Lane <[email protected]> wrote:
>
> What would be
> lost if we drop it?
I looked into this a bit. Removing that bit of code, the only regression test changes for "check-world" are the expected ones, with nothing else breaking. Running installcheck+pg_upgrade to the patched version of HEAD from each of versions 11, 12, 13 and 14 doesn't turn up anything untoward. The change I used (for reference) is attached:
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[application/octet-stream] v1-0001-WIP-Removing-role-self-administration.patch.WIP (4.3K, ../../[email protected]/2-v1-0001-WIP-Removing-role-self-administration.patch.WIP)
download
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 20:51 Robert Haas <[email protected]>
parent: Mark Dilger <[email protected]>
0 siblings, 2 replies; 41+ messages in thread
From: Robert Haas @ 2022-03-09 20:51 UTC (permalink / raw)
To: Mark Dilger <[email protected]>; +Cc: Tom Lane <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Mon, Mar 7, 2022 at 11:14 PM Mark Dilger
<[email protected]> wrote:
> > On Mar 7, 2022, at 12:16 PM, Tom Lane <[email protected]> wrote:
> > What would be
> > lost if we drop it?
>
> I looked into this a bit. Removing that bit of code, the only regression test changes for "check-world" are the expected ones, with nothing else breaking. Running installcheck+pg_upgrade to the patched version of HEAD from each of versions 11, 12, 13 and 14 doesn't turn up anything untoward.
I looked into this a bit, too. I attach a draft patch for removing the
self-admin exception.
I found that having is_admin_of_role() return true matters in three
ways: (1) It lets you grant membership in the role to some other role.
(2) It lets you revoke membership in the role from some other role.
(3) It changes the return value of pg_role_aclcheck(), which is used
in the implementation of various SQL-callable functions all invoked
via the name pg_has_role(). We've mostly been discussing (2) as an
issue, but (1) and (3) are pretty interesting too. Regarding (3),
there is a comment in the code indicating that Noah considered the
self-admin exception something of a wart as far as pg_has_role() is
concerned. As to (1), I discovered that today you can do this:
rhaas=# create user foo;
CREATE ROLE
rhaas=# create user bar;
CREATE ROLE
rhaas=# \q
[rhaas ~]$ psql -U foo rhaas
psql (15devel)
Type "help" for help.
rhaas=> grant foo to bar with admin option;
GRANT ROLE
I don't know why I didn't realize that before. It's a natural result
of treating the logged-in user as if they had admin option. But it's
weird that you can't even be granted WITH ADMIN OPTION on your own
login role, but at the same time without having it you can grant it to
someone else!
I believe there are three other points worth some consideration here.
First, in the course of my investigation I re-discovered what Tom
already did a good job articulating:
tgl> Having said that, one thing that I find fishy is that it's not clear
tgl> where the admin privilege for a role originates. After "CREATE ROLE
tgl> alice", alice has no members, therefore none that have admin privilege,
tgl> therefore the only way that the first member could be added is via
tgl> superuser deus ex machina. This does not seem clean.
I agree with that, but I don't think it's a sufficient reason for
keeping the self-admin exception, because the same problem exists for
non-login roles. I don't even think it's the right idea conceptually
to suppose that the power to administer a role originates from the
role itself. If that were so, then it would be inherited by all
members of the role along with all the rest of the role's privileges,
which is so clearly not right that we've already prohibited a role
from having WITH ADMIN OPTION on itself. In my opinion, the right to
administer a role - regardless of whether or not it is a login role -
most naturally vests in the role that created it, or something in that
direction at least, if not that exact thing. Today, that means the
superuser or a CREATEROLE user who could hack superuser if they
wished. In the future, I hope for other alternatives, as recently
argued on other threads. But we need not resolve the question of how
that should work exactly in order to agree (as I hope we do) that
doubling down on the self-administration exception is not the answer.
Second, it occured to me to wonder what implications a change like
this might have for dump and restore. If privilege restoration somehow
relied on this behavior, then we'd have a problem. But I don't think
it does, because (a) pg_dump can SET ROLE but can't change the session
user without reconnecting, so it's unclear how we could be relying on
it; (b) it wouldn't work for non-login roles, and it's unlikely that
we would treat login and non-login roles different in terms of
restoring privileges, and (c) when I execute the example shown above
and then run pg_dump, there's no attempt to change the current user,
it just dumps "GRANT foo TO bar WITH ADMIN OPTION GRANTED BY foo".
Third, it occurred to me to wonder whether some users might be using
and relying upon this behavior. It's certainly possible, and it does
suck that we'd be removing it without providing a workable substitute.
But it's probably not a LOT of users because most people who have
commented on this topic on this mailing list seem to find granting
membership in a login role a super-weird thing to do, because a lot of
people really seem to want every role to be a user or a group, and a
login role with members feels like it's blurring that line. I'm
inclined to think that the small number of people who may be unhappy
is an acceptable price to pay for removing this wart, but it's a
judgement call and if someone has information to suggest that I'm
wrong, it'd be good to hear about that.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] remove-self-admin.patch (6.5K, ../../CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com/2-remove-self-admin.patch)
download | inline diff:
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index a897712de2..8c4edd9b0a 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -251,11 +251,10 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
in turn grant membership in the role to others, and revoke membership
in the role as well. Without the admin option, ordinary users cannot
do that. A role is not considered to hold <literal>WITH ADMIN
- OPTION</literal> on itself, but it may grant or revoke membership in
- itself from a database session where the session user matches the
- role. Database superusers can grant or revoke membership in any role
- to anyone. Roles having <literal>CREATEROLE</literal> privilege can grant
- or revoke membership in any role that is not a superuser.
+ OPTION</literal> on itself. Database superusers can grant or revoke
+ membership in any role to anyone. Roles having
+ <literal>CREATEROLE</literal> privilege can grant or revoke membership
+ in any role that is not a superuser.
</para>
<para>
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index f9d3c1246b..c263f6c8b9 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1425,11 +1425,6 @@ AddRoleMems(const char *rolename, Oid roleid,
* The role membership grantor of record has little significance at
* present. Nonetheless, inasmuch as users might look to it for a crude
* audit trail, let only superusers impute the grant to a third party.
- *
- * Before lifting this restriction, give the member == role case of
- * is_admin_of_role() a fresh look. Ensure that the current role cannot
- * use an explicit grantor specification to take advantage of the session
- * user's self-admin right.
*/
if (grantorId != GetUserId() && !superuser())
ereport(ERROR,
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 0a16f8156c..894da4891a 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -4619,11 +4619,6 @@ pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode)
{
if (mode & ACL_GRANT_OPTION_FOR(ACL_CREATE))
{
- /*
- * XXX For roleid == role_oid, is_admin_of_role() also examines the
- * session and call stack. That suits two-argument pg_has_role(), but
- * it gives the three-argument version a lamentable whimsy.
- */
if (is_admin_of_role(roleid, role_oid))
return ACLCHECK_OK;
}
@@ -4935,38 +4930,12 @@ is_admin_of_role(Oid member, Oid role)
if (superuser_arg(member))
return true;
+ /*
+ * A role cannot have WITH ADMIN OPTION on itself, because that would
+ * imply a membership loop.
+ */
if (member == role)
-
- /*
- * A role can admin itself when it matches the session user and we're
- * outside any security-restricted operation, SECURITY DEFINER or
- * similar context. SQL-standard roles cannot self-admin. However,
- * SQL-standard users are distinct from roles, and they are not
- * grantable like roles: PostgreSQL's role-user duality extends the
- * standard. Checking for a session user match has the effect of
- * letting a role self-admin only when it's conspicuously behaving
- * like a user. Note that allowing self-admin under a mere SET ROLE
- * would make WITH ADMIN OPTION largely irrelevant; any member could
- * SET ROLE to issue the otherwise-forbidden command.
- *
- * Withholding self-admin in a security-restricted operation prevents
- * object owners from harnessing the session user identity during
- * administrative maintenance. Suppose Alice owns a database, has
- * issued "GRANT alice TO bob", and runs a daily ANALYZE. Bob creates
- * an alice-owned SECURITY DEFINER function that issues "REVOKE alice
- * FROM carol". If he creates an expression index calling that
- * function, Alice will attempt the REVOKE during each ANALYZE.
- * Checking InSecurityRestrictedOperation() thwarts that attack.
- *
- * Withholding self-admin in SECURITY DEFINER functions makes their
- * behavior independent of the calling user. There's no security or
- * SQL-standard-conformance need for that restriction, though.
- *
- * A role cannot have actual WITH ADMIN OPTION on itself, because that
- * would imply a membership loop. Therefore, we're done either way.
- */
- return member == GetSessionUserId() &&
- !InLocalUserIdChange() && !InSecurityRestrictedOperation();
+ return false;
(void) roles_is_member_of(member, ROLERECURSE_MEMBERS, role, &result);
return result;
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 291e21d7a6..cc3b6e116c 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1555,14 +1555,8 @@ SET ROLE regress_priv_group2;
GRANT regress_priv_group2 TO regress_priv_user5; -- fails: SET ROLE did not help
ERROR: must have admin option on role "regress_priv_group2"
SET SESSION AUTHORIZATION regress_priv_group2;
-GRANT regress_priv_group2 TO regress_priv_user5; -- ok: a role can self-admin
-NOTICE: role "regress_priv_user5" is already a member of role "regress_priv_group2"
-CREATE FUNCTION dogrant_fails() RETURNS void LANGUAGE sql SECURITY DEFINER AS
- 'GRANT regress_priv_group2 TO regress_priv_user5';
-SELECT dogrant_fails(); -- fails: no self-admin in SECURITY DEFINER
+GRANT regress_priv_group2 TO regress_priv_user5; -- fails: no self-admin
ERROR: must have admin option on role "regress_priv_group2"
-CONTEXT: SQL function "dogrant_fails" statement 1
-DROP FUNCTION dogrant_fails();
SET SESSION AUTHORIZATION regress_priv_user4;
DROP FUNCTION dogrant_ok();
REVOKE regress_priv_group2 FROM regress_priv_user5;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index c8c545b64c..49bf1f2a62 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -981,11 +981,7 @@ SET ROLE regress_priv_group2;
GRANT regress_priv_group2 TO regress_priv_user5; -- fails: SET ROLE did not help
SET SESSION AUTHORIZATION regress_priv_group2;
-GRANT regress_priv_group2 TO regress_priv_user5; -- ok: a role can self-admin
-CREATE FUNCTION dogrant_fails() RETURNS void LANGUAGE sql SECURITY DEFINER AS
- 'GRANT regress_priv_group2 TO regress_priv_user5';
-SELECT dogrant_fails(); -- fails: no self-admin in SECURITY DEFINER
-DROP FUNCTION dogrant_fails();
+GRANT regress_priv_group2 TO regress_priv_user5; -- fails: no self-admin
SET SESSION AUTHORIZATION regress_priv_user4;
DROP FUNCTION dogrant_ok();
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 21:01 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
1 sibling, 2 replies; 41+ messages in thread
From: Tom Lane @ 2022-03-09 21:01 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Mark Dilger <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Robert Haas <[email protected]> writes:
> On Mar 7, 2022, at 12:16 PM, Tom Lane <[email protected]> wrote:
> tgl> Having said that, one thing that I find fishy is that it's not clear
> tgl> where the admin privilege for a role originates. After "CREATE ROLE
> tgl> alice", alice has no members, therefore none that have admin privilege,
> tgl> therefore the only way that the first member could be added is via
> tgl> superuser deus ex machina. This does not seem clean.
> I agree with that, but I don't think it's a sufficient reason for
> keeping the self-admin exception, because the same problem exists for
> non-login roles. I don't even think it's the right idea conceptually
> to suppose that the power to administer a role originates from the
> role itself.
Actually, that's the same thing I was trying to say. But if it doesn't
originate from the role itself, where does it originate from?
> In my opinion, the right to
> administer a role - regardless of whether or not it is a login role -
> most naturally vests in the role that created it, or something in that
> direction at least, if not that exact thing.
This seems like a reasonable answer to me too: the creating role has admin
option implicitly, and can then choose to grant that to other roles.
Obviously some work needs to be done to make that happen (and we should
see whether the SQL spec has some different idea).
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 21:15 Robert Haas <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 2 replies; 41+ messages in thread
From: Robert Haas @ 2022-03-09 21:15 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Mark Dilger <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Wed, Mar 9, 2022 at 4:01 PM Tom Lane <[email protected]> wrote:
> > In my opinion, the right to
> > administer a role - regardless of whether or not it is a login role -
> > most naturally vests in the role that created it, or something in that
> > direction at least, if not that exact thing.
>
> This seems like a reasonable answer to me too: the creating role has admin
> option implicitly, and can then choose to grant that to other roles.
> Obviously some work needs to be done to make that happen (and we should
> see whether the SQL spec has some different idea).
Well, the problem is that as far as I can see, the admin option is an
optional feature of membership. You can grant someone membership
without admin option, or with admin option, but you can't grant them
the admin option without membership, just like you can't purchase an
upgrade to first class without the underlying plane ticket. What would
the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
BUT WITHOUT MEMBERSHIP? Yikes.
But do we really have to solve this problem before we can clean up
this session exception? I hope not, because I think that's a much
bigger can of worms than this is.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 21:20 Stephen Frost <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 0 replies; 41+ messages in thread
From: Stephen Frost @ 2022-03-09 21:20 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Mark Dilger <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Greetings,
* Tom Lane ([email protected]) wrote:
> Robert Haas <[email protected]> writes:
> > On Mar 7, 2022, at 12:16 PM, Tom Lane <[email protected]> wrote:
> > tgl> Having said that, one thing that I find fishy is that it's not clear
> > tgl> where the admin privilege for a role originates. After "CREATE ROLE
> > tgl> alice", alice has no members, therefore none that have admin privilege,
> > tgl> therefore the only way that the first member could be added is via
> > tgl> superuser deus ex machina. This does not seem clean.
>
> > I agree with that, but I don't think it's a sufficient reason for
> > keeping the self-admin exception, because the same problem exists for
> > non-login roles. I don't even think it's the right idea conceptually
> > to suppose that the power to administer a role originates from the
> > role itself.
>
> Actually, that's the same thing I was trying to say. But if it doesn't
> originate from the role itself, where does it originate from?
>
> > In my opinion, the right to
> > administer a role - regardless of whether or not it is a login role -
> > most naturally vests in the role that created it, or something in that
> > direction at least, if not that exact thing.
>
> This seems like a reasonable answer to me too: the creating role has admin
> option implicitly, and can then choose to grant that to other roles.
I agree that this has some appeal, but it's not desirable in all cases
and so I wouldn't want it to be fully baked into the system ala the role
'owner' concept.
> Obviously some work needs to be done to make that happen (and we should
> see whether the SQL spec has some different idea).
Agreed on this, though I don't recall it having much to say on it.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 21:23 Stephen Frost <[email protected]>
parent: Robert Haas <[email protected]>
1 sibling, 0 replies; 41+ messages in thread
From: Stephen Frost @ 2022-03-09 21:23 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Mark Dilger <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Greetings,
* Robert Haas ([email protected]) wrote:
> On Wed, Mar 9, 2022 at 4:01 PM Tom Lane <[email protected]> wrote:
> > > In my opinion, the right to
> > > administer a role - regardless of whether or not it is a login role -
> > > most naturally vests in the role that created it, or something in that
> > > direction at least, if not that exact thing.
> >
> > This seems like a reasonable answer to me too: the creating role has admin
> > option implicitly, and can then choose to grant that to other roles.
> > Obviously some work needs to be done to make that happen (and we should
> > see whether the SQL spec has some different idea).
>
> Well, the problem is that as far as I can see, the admin option is an
> optional feature of membership. You can grant someone membership
> without admin option, or with admin option, but you can't grant them
> the admin option without membership, just like you can't purchase an
> upgrade to first class without the underlying plane ticket. What would
> the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
> BUT WITHOUT MEMBERSHIP? Yikes.
I've been meaning to reply to your other email regarding this, but I
don't really agree that the syntax ends up being so terrible or
difficult to deal with, considering we have these same general things
for ALTER ROLE already and there hasn't been all that much complaining.
That is, we have LOGIN and NOLOGIN, CREATEROLE and NOCREATEROLE, and we
could have MEMBERSHIP and NOMEMBERSHIP pretty easily here if we wanted
to.
> But do we really have to solve this problem before we can clean up
> this session exception? I hope not, because I think that's a much
> bigger can of worms than this is.
I do believe we can deal with the above independently and at a later
time and go ahead and clean up the session excepton bit without dealing
with the above at the same time.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 21:24 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
1 sibling, 0 replies; 41+ messages in thread
From: Tom Lane @ 2022-03-09 21:24 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Mark Dilger <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
I wrote:
> This seems like a reasonable answer to me too: the creating role has admin
> option implicitly, and can then choose to grant that to other roles.
> Obviously some work needs to be done to make that happen (and we should
> see whether the SQL spec has some different idea).
Ah, here we go: it's buried under CREATE ROLE. SQL:2021 12.4 <role
definition> saith that when role A executes CREATE ROLE <role name>,
then
1) A grantable role authorization descriptor is created whose role name
is <role name>, whose grantor is "_SYSTEM", and whose grantee is A.
Since nobody is _SYSTEM, this grant can't be deleted except by dropping
the new role (or, maybe, dropping A?). So that has nearly the same
end result as "the creating role has admin option implicitly". The main
difference I can see is that it also means the creating role is a *member*
implicitly, which is something I'd argue we don't want to enforce. This
is analogous to the way we let an object owner revoke her own ordinary
permissions, which the SQL model doesn't allow since those permissions
were granted to her by _SYSTEM.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 21:31 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
1 sibling, 2 replies; 41+ messages in thread
From: Tom Lane @ 2022-03-09 21:31 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Mark Dilger <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Robert Haas <[email protected]> writes:
> Well, the problem is that as far as I can see, the admin option is an
> optional feature of membership. You can grant someone membership
> without admin option, or with admin option, but you can't grant them
> the admin option without membership, just like you can't purchase an
> upgrade to first class without the underlying plane ticket. What would
> the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
> BUT WITHOUT MEMBERSHIP? Yikes.
I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.
Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?
> But do we really have to solve this problem before we can clean up
> this session exception?
I think we need a plan for where we're going. I don't see "clean up
the session exception" as an end in itself; it's part of re-examining
how all of this ought to work. I don't say that we have to have a
complete patch right away, only that we need a coherent end goal.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 22:00 David G. Johnston <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 41+ messages in thread
From: David G. Johnston @ 2022-03-09 22:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Mark Dilger <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Wed, Mar 9, 2022 at 2:31 PM Tom Lane <[email protected]> wrote:
> Robert Haas <[email protected]> writes:
> > Well, the problem is that as far as I can see, the admin option is an
> > optional feature of membership. You can grant someone membership
> > without admin option, or with admin option, but you can't grant them
> > the admin option without membership, just like you can't purchase an
> > upgrade to first class without the underlying plane ticket. What would
> > the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
> > BUT WITHOUT MEMBERSHIP? Yikes.
>
> I don't think we need syntax to describe it. As I just said in my
> other reply, we have a perfectly good precedent for this already
> in ordinary object permissions. That is: an object owner always,
> implicitly, has GRANT OPTION for all the object's privileges, even
> if she revoked the corresponding plain privilege from herself.
>
So CREATE ROLE will assign ownership of AND membership in the newly created
role to the session_user UNLESS the OWNER clause is present in which case
the named role, so long as the session_user can SET ROLE to the named role,
becomes the owner & member. Subsequent to that the owner can issue: REVOKE
new_role FROM role_name where role_name is again the session_user role or
one that can be SET ROLE to.
> Yeah, this does mean that we're effectively deciding that the creator
> of a role is its owner. What's the problem with that?
>
I'm fine with this. It does introduce an OWNER concept to roles and so at
minimum we need to add:
ALTER ROLE foo OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER |
SESSION_USER }
And similar for CREATE ROLE
And keep the USER alias commands in sync.
GROUP commands are only present for backward compatibility and so don't get
updated with new features by design.
Obviously a superuser can change ownership.
Playing with table ownership I find this behavior:
-- superuser
CREATE ROLE tblowner;
CREATE TABLE tblowner_test (id serial primary key);
ALTER TABLE tblowner_test OWNER TO tblowner;
CREATE ROLE boss;
GRANT boss TO tblowner;
SET SESSION AUTHORIZATION tblowner;
ALTER TABLE tblowner_test OWNER TO boss; --works
So tblowner can push their ownership attribute to any group they are a
member of. Is that the behavior we want for roles as well?
David J.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-09 22:35 Tom Lane <[email protected]>
parent: David G. Johnston <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Tom Lane @ 2022-03-09 22:35 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Robert Haas <[email protected]>; Mark Dilger <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
"David G. Johnston" <[email protected]> writes:
> So CREATE ROLE will assign ownership of AND membership in the newly created
> role to the session_user
I would NOT have it automatically assign membership in the new role,
even though the SQL spec says so. We've not done that historically
and it doesn't seem desirable. In particular, it's *really* not
desirable for a user (role with LOGIN).
> I'm fine with this. It does introduce an OWNER concept to roles and so at
> minimum we need to add:
> ALTER ROLE foo OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER |
> SESSION_USER }
Agreed.
regards, tom lane
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 14:46 Robert Haas <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 41+ messages in thread
From: Robert Haas @ 2022-03-10 14:46 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Mark Dilger <[email protected]>; David G. Johnston <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Wed, Mar 9, 2022 at 4:31 PM Tom Lane <[email protected]> wrote:
> I don't think we need syntax to describe it. As I just said in my
> other reply, we have a perfectly good precedent for this already
> in ordinary object permissions. That is: an object owner always,
> implicitly, has GRANT OPTION for all the object's privileges, even
> if she revoked the corresponding plain privilege from herself.
>
> Yeah, this does mean that we're effectively deciding that the creator
> of a role is its owner. What's the problem with that?
I don't think that's entirely the wrong concept, but it doesn't make a
lot of sense in a world where the creator has to be a superuser. If
alice, bob, and charlie are superusers who take turns creating new
users, and then we let charlie go due to budget cuts, forcing alice
and bob to change the owner of all the users he created to some other
superuser as a condition of dropping his account is a waste of
everyone's time. They can do exactly the same things to every account
on the system after we change the role owner as before.
But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
generally agreed to be broken right now, and if you don't agree with
that, consider that it can grant pg_execute_server_programs to a
newly-created account and then explain to me how it's functionally
different from superuser. The whole area needs a rethink. I believe
everyone involved in the discussion on the other threads agrees that
some reform of CREATEROLE is necessary, and more generally with the
idea that it's useful for non-superusers to be able to create roles.
But the reasons why people want that vary.
I want that because I want mini-superusers, where alice can administer
the users that alice creates just as if she were a superuser,
including having their permissions implicitly and dropping them when
she wants them gone, but where alice cannot break out to the operating
system as a true superuser could do. I want this because the lack of
meaningful privilege separation that led to CVE-2019-9193 being filed
spuriously is a very real problem. It's a thing a lot of people want,
and I want to give it to them. David Steele, on the other hand, wants
to build a user-creating bot that can create accounts but otherwise
conforms to the principle of least privilege: the bot can stand up
accounts, can grant them membership in a defined set of groups, but
cannot exercise the privileges of those accounts (or hack superuser
either). Other people may well want other things.
And that's why I'm not sure it's really the right idea to say that we
don't need syntax for this admin-without-member concept. If we just
want to bolt role ownership onto the existing framework without really
changing anything else, we can do that without extra syntax and, as
you say here, make it an implicit property of role ownership. But I
don't see that as has having much value; we just end up with a bunch
of superuser owners. Whatever. Now Stephen made the argument that we
ought to actually have admin-without-member as a first class concept,
something that could be assigned to arbitrary users. Actually, I think
he wanted it even more fine grained with that. And I think that could
make the concept a lot more useful, but then it needs some kind of
understandable syntax.
There's a lot of moving parts here. It's not just about coming up with
something that sounds generally logical, but about creating a system
that has some real-world utility.
> > But do we really have to solve this problem before we can clean up
> > this session exception?
>
> I think we need a plan for where we're going. I don't see "clean up
> the session exception" as an end in itself; it's part of re-examining
> how all of this ought to work. I don't say that we have to have a
> complete patch right away, only that we need a coherent end goal.
I'd like to have a plan, too, but if this behavior is accidental, I
still think we can remove it without making big decisions about future
direction. The perfect is the enemy of the good.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 15:56 David G. Johnston <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 2 replies; 41+ messages in thread
From: David G. Johnston @ 2022-03-10 15:56 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Mark Dilger <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <[email protected]> wrote:
> On Wed, Mar 9, 2022 at 4:31 PM Tom Lane <[email protected]> wrote:
> > I don't think we need syntax to describe it. As I just said in my
> > other reply, we have a perfectly good precedent for this already
> > in ordinary object permissions. That is: an object owner always,
> > implicitly, has GRANT OPTION for all the object's privileges, even
> > if she revoked the corresponding plain privilege from herself.
> >
> > Yeah, this does mean that we're effectively deciding that the creator
> > of a role is its owner. What's the problem with that?
>
> I don't think that's entirely the wrong concept, but it doesn't make a
> lot of sense in a world where the creator has to be a superuser. If
> alice, bob, and charlie are superusers who take turns creating new
> users, and then we let charlie go due to budget cuts, forcing alice
> and bob to change the owner of all the users he created to some other
> superuser as a condition of dropping his account is a waste of
> everyone's time. They can do exactly the same things to every account
> on the system after we change the role owner as before.
>
Then maybe we should just implement the idea that if a superuser would
become the owner we instead substitute in the bootstrap user. Or give the
DBA the choice whether they want to retain knowledge of specific roles -
and thus are willing to accept the "waste of time".
> But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
> generally agreed to be broken right now, and if you don't agree with
> that, consider that it can grant pg_execute_server_programs to a
> newly-created account and then explain to me how it's functionally
> different from superuser.
CREATEROLE has long been defined as basically having "with admin option" on
every role in the system. The failure to special-case the roles that grant
different aspects of superuser-ness to its members doesn't make CREATEROLE
itself broken, it makes the implementation of pg_execute_server_programs
broken. Only superusers should be considered to have with admin option on
these roles. They can delegate through the usual membership+admin mechanism
to a CREATEROLE role if they desire.
> The whole area needs a rethink. I believe
> everyone involved in the discussion on the other threads agrees that
> some reform of CREATEROLE is necessary, and more generally with the
> idea that it's useful for non-superusers to be able to create roles.
>
As the documentation says, using SUPERUSER for day-to-day administration is
contrary to good security practices. Role management is considered to be a
day-to-day administration activity. I agree with this principle. It was
designed to neither be a superuser nor grant superuser, so removing the
ability to grant the pg_* role memberships remains consistent with its
original intent.
> I want that because I want mini-superusers, where alice can administer
> the users that alice creates just as if she were a superuser,
> including having their permissions implicitly and dropping them when
> she wants them gone, but where alice cannot break out to the operating
> system as a true superuser could do.
CREATEROLE (once the pg_* with admin rules are fixed) + Ownership and rules
restricting interfering with another role's objects (unless superuser)
seems to handle this.
> the bot can stand up
> accounts, can grant them membership in a defined set of groups, but
> cannot exercise the privileges of those accounts (or hack superuser
> either).
The bot should be provided a security definer procedure that encapsulates
all of this rather than us trying to hack the permission system. This
isn't a user permission concern, it is an unauthorized privilege escalation
concern. Anyone with the bot's credentials can trivially overcome the
third restriction by creating a role with the desired membership and then
logging in as that role - and there is nothing the system can do to prevent
that while also allowing the other two permissions.
> And that's why I'm not sure it's really the right idea to say that we
> don't need syntax for this admin-without-member concept.
We already have this syntax in the form of CREATEROLE. But we do need a
fix, just on the group side. We need a way to define a group as having no
ADMINS.
ALTER ROLE pg_superuser WITH [NO] ADMIN;
Then adding a role membership including the WITH ADMIN OPTION can be
rejected, as can the non-superuser situation. Setting WITH NO ADMIN should
fail if any existing members have admin. You must be a superuser to
execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
new pg_* role that grants this ability (and maybe some others) for use by a
backup/restore user.
Or just special-case pg_* roles.
The advantage of exposing this to the DBA is that they can then package
pg_* roles into a custom group and still have the benefit of superuser only
administration. In the special-case implementation the presence of a pg_*
role in a group hierarchy would then preclude a non-superuser from having
admin on the entire tree (the pg_* roles are all roots, or in the case of
pg_monitor, directly emanate from a root role).
David J.
David J.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 16:19 Stephen Frost <[email protected]>
parent: David G. Johnston <[email protected]>
1 sibling, 2 replies; 41+ messages in thread
From: Stephen Frost @ 2022-03-10 16:19 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Mark Dilger <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Greetings,
* David G. Johnston ([email protected]) wrote:
> On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <[email protected]> wrote:
> > On Wed, Mar 9, 2022 at 4:31 PM Tom Lane <[email protected]> wrote:
> > > I don't think we need syntax to describe it. As I just said in my
> > > other reply, we have a perfectly good precedent for this already
> > > in ordinary object permissions. That is: an object owner always,
> > > implicitly, has GRANT OPTION for all the object's privileges, even
> > > if she revoked the corresponding plain privilege from herself.
> > >
> > > Yeah, this does mean that we're effectively deciding that the creator
> > > of a role is its owner. What's the problem with that?
> >
> > I don't think that's entirely the wrong concept, but it doesn't make a
> > lot of sense in a world where the creator has to be a superuser. If
> > alice, bob, and charlie are superusers who take turns creating new
> > users, and then we let charlie go due to budget cuts, forcing alice
> > and bob to change the owner of all the users he created to some other
> > superuser as a condition of dropping his account is a waste of
> > everyone's time. They can do exactly the same things to every account
> > on the system after we change the role owner as before.
>
> Then maybe we should just implement the idea that if a superuser would
> become the owner we instead substitute in the bootstrap user. Or give the
> DBA the choice whether they want to retain knowledge of specific roles -
> and thus are willing to accept the "waste of time".
This doesn't strike me as going in the right direction. Falling back to
the bootstrap superuser is generally a hack and not a great one. I'll
also point out that the SQL spec hasn't got a concept of role ownership
either.
> > But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
> > generally agreed to be broken right now, and if you don't agree with
> > that, consider that it can grant pg_execute_server_programs to a
> > newly-created account and then explain to me how it's functionally
> > different from superuser.
>
> CREATEROLE has long been defined as basically having "with admin option" on
> every role in the system. The failure to special-case the roles that grant
> different aspects of superuser-ness to its members doesn't make CREATEROLE
> itself broken, it makes the implementation of pg_execute_server_programs
> broken. Only superusers should be considered to have with admin option on
> these roles. They can delegate through the usual membership+admin mechanism
> to a CREATEROLE role if they desire.
No, CREATEROLE having admin option on every role in the system is broken
and always has been. It's not just an issue for predefined roles like
pg_execute_server_program, it's an issue for any role that could become
a superuser either directly or indirectly and that extends beyond the
predefined ones. As this issue with CREATEROLE existed way before
predefined roles were added to PG, claiming that it's an issue with
predefined roles doesn't make a bit of sense.
> > The whole area needs a rethink. I believe
> > everyone involved in the discussion on the other threads agrees that
> > some reform of CREATEROLE is necessary, and more generally with the
> > idea that it's useful for non-superusers to be able to create roles.
>
> As the documentation says, using SUPERUSER for day-to-day administration is
> contrary to good security practices. Role management is considered to be a
> day-to-day administration activity. I agree with this principle. It was
> designed to neither be a superuser nor grant superuser, so removing the
> ability to grant the pg_* role memberships remains consistent with its
> original intent.
That would not be sufficient to make CREATEROLE safe. Far, far from it.
> > I want that because I want mini-superusers, where alice can administer
> > the users that alice creates just as if she were a superuser,
> > including having their permissions implicitly and dropping them when
> > she wants them gone, but where alice cannot break out to the operating
> > system as a true superuser could do.
>
> CREATEROLE (once the pg_* with admin rules are fixed) + Ownership and rules
> restricting interfering with another role's objects (unless superuser)
> seems to handle this.
This is not sufficient- roles can be not-superuser themselves but have
the ability to become superuser if GRANT'd a superuser role and
therefore we can't have a system where CREATEROLE allows arbitrary
GRANT'ing of roles to each other. I'm a bit confused too as anything
where we are curtailing what CREATEROLE roles are able to do in a manner
that means they're only able to modify some subset of roles should
equally apply to predefined roles too- that is, CREATEROLE shouldn't be
the determining factor in the question of if a role can GRANT a
predefined (or any other role) to some other role- that should be
governed by the admin option on that role, and that should work exactly
the same for predefined roles as it does for any other.
I disagree that ownership is needed that's not what the spec calls for
either. What we need is more flexibility when it comes to the
relationships which are allowed to be created between roles and what
privileges come with them. To that end, I'd argue that we should be
extending pg_auth_members, first by separating out membership itself
into an explicitly tracked attribute (instead of being implicit in the
existance of a row in the table) and then adding on what other
privileges we see fit to add, such as the ability to DROP a role. We
do need to remove the ability for a role who hasn't been explicitly
given the admin right on another role to modify that role's membership
too, as was originally proposed here. This also seems to more closely
follow the spec's expectation, something that role ownership doesn't.
> > the bot can stand up
> > accounts, can grant them membership in a defined set of groups, but
> > cannot exercise the privileges of those accounts (or hack superuser
> > either).
>
> The bot should be provided a security definer procedure that encapsulates
> all of this rather than us trying to hack the permission system. This
> isn't a user permission concern, it is an unauthorized privilege escalation
> concern. Anyone with the bot's credentials can trivially overcome the
> third restriction by creating a role with the desired membership and then
> logging in as that role - and there is nothing the system can do to prevent
> that while also allowing the other two permissions.
Falling back to security definer functions may be one approach but it's
not a great one and it only works if it's possible to end up with the
catalogs having what is actually desired- for example, ADMIN option
without membership isn't something the catalogs today can understand
because existance in pg_auth_members implies membership and you can't
have ADMIN without having that row. The same issue would exist with
ownership if ownership implied the same- that's not improving things.
> > And that's why I'm not sure it's really the right idea to say that we
> > don't need syntax for this admin-without-member concept.
>
> We already have this syntax in the form of CREATEROLE. But we do need a
> fix, just on the group side. We need a way to define a group as having no
> ADMINS.
We don't have this syntax today nor do we have a way to store such a
concept in the catalogs either, so I'm pretty baffled by this. Defining
a group without admins is, in fact, what we actually have support for
today in the catalogs- it's just a case where there aren't any rows in
pg_auth_members which have 'admin_option' as true. The opposite is what
we're talking about here- rows which have 'admin_option' as true but
don't have membership, and that can't be the case today because
existance in the table itself implies membership.
> ALTER ROLE pg_superuser WITH [NO] ADMIN;
>
> Then adding a role membership including the WITH ADMIN OPTION can be
> rejected, as can the non-superuser situation. Setting WITH NO ADMIN should
> fail if any existing members have admin. You must be a superuser to
> execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
> new pg_* role that grants this ability (and maybe some others) for use by a
> backup/restore user.
I'm not following this in general or how it helps. Surely we don't want
to limit WITH ADMIN to superusers. As for if we should migrate
CREATEROLE to a new predefined role, maybe, but that seems like a
different question.
> Or just special-case pg_* roles.
As I hopefully made clear above, this isn't actually a solution, nor do
pg_* roles need to be treated somehow differently in this aspect.
> The advantage of exposing this to the DBA is that they can then package
> pg_* roles into a custom group and still have the benefit of superuser only
> administration. In the special-case implementation the presence of a pg_*
> role in a group hierarchy would then preclude a non-superuser from having
> admin on the entire tree (the pg_* roles are all roots, or in the case of
> pg_monitor, directly emanate from a root role).
We are very much trying to move away from 'superuser only
administration'.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 16:26 Mark Dilger <[email protected]>
parent: David G. Johnston <[email protected]>
1 sibling, 0 replies; 41+ messages in thread
From: Mark Dilger @ 2022-03-10 16:26 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
> On Mar 10, 2022, at 7:56 AM, David G. Johnston <[email protected]> wrote:
>
>
> I want that because I want mini-superusers, where alice can administer
> the users that alice creates just as if she were a superuser,
> including having their permissions implicitly and dropping them when
> she wants them gone, but where alice cannot break out to the operating
> system as a true superuser could do.
>
> CREATEROLE (once the pg_* with admin rules are fixed) + Ownership and rules restricting interfering with another role's objects (unless superuser) seems to handle this.
What if one of alice's subordinates also owns roles? Can alice interfere with *that* role's objects? I don't see that a simple rule restricting roles from interfering with another role's objects is quite enough. That raises the question of whether role ownership is transitive, and whether we need a concept similar to inherit/noinherit for ownership.
There is also the problem that CREATEROLE currently allows a set of privileges to be granted to created roles, and that set of privileges is hard-coded. You've suggested changing the hard-coded rules to remove pg_* roles from the list of grantable privileges, but that's still an inflexible set of hardcoded privileges. Wouldn't it make more sense for the grantor to need GRANT OPTION on any privilege they give to roles they create?
> the bot can stand up
> accounts, can grant them membership in a defined set of groups, but
> cannot exercise the privileges of those accounts (or hack superuser
> either).
>
> The bot should be provided a security definer procedure that encapsulates all of this rather than us trying to hack the permission system. This isn't a user permission concern, it is an unauthorized privilege escalation concern. Anyone with the bot's credentials can trivially overcome the third restriction by creating a role with the desired membership and then logging in as that role - and there is nothing the system can do to prevent that while also allowing the other two permissions.
Doesn't this assume password authentication? If the server uses ldap authentication, for example, wouldn't the bot need valid ldap credentials for at least one user for this attack to work? And if CREATEROLE has been made more configurable, wouldn't the bot only be able to grant that ldap user the limited set of privileges that the bot's database user has been granted ADMIN OPTION for?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 17:11 Robert Haas <[email protected]>
parent: Stephen Frost <[email protected]>
1 sibling, 1 reply; 41+ messages in thread
From: Robert Haas @ 2022-03-10 17:11 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: David G. Johnston <[email protected]>; Tom Lane <[email protected]>; Mark Dilger <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Thu, Mar 10, 2022 at 11:19 AM Stephen Frost <[email protected]> wrote:
> I disagree that ownership is needed that's not what the spec calls for
> either. What we need is more flexibility when it comes to the
> relationships which are allowed to be created between roles and what
> privileges come with them. To that end, I'd argue that we should be
> extending pg_auth_members, first by separating out membership itself
> into an explicitly tracked attribute (instead of being implicit in the
> existance of a row in the table) and then adding on what other
> privileges we see fit to add, such as the ability to DROP a role. We
> do need to remove the ability for a role who hasn't been explicitly
> given the admin right on another role to modify that role's membership
> too, as was originally proposed here. This also seems to more closely
> follow the spec's expectation, something that role ownership doesn't.
I do not have a problem with more fine-grained kinds of authorization
even though I think there are syntactic issues to work out, but I
strongly disagree with the idea that we can't or shouldn't also have
role ownership. Marc invented it. Now Tom has invented it
independently. All sorts of other objects have it already. Trying to
make it out like this is some kind of kooky idea is not believable.
Yeah, it's not the most sophisticated or elegant model and that's why
it's good for us to also have other things, but for simple cases it is
easy to understand and works great.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 17:26 David G. Johnston <[email protected]>
parent: Stephen Frost <[email protected]>
1 sibling, 1 reply; 41+ messages in thread
From: David G. Johnston @ 2022-03-10 17:26 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Mark Dilger <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <[email protected]> wrote:
> Greetings,
>
> * David G. Johnston ([email protected]) wrote:
> > On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <[email protected]>
> wrote:
> > > On Wed, Mar 9, 2022 at 4:31 PM Tom Lane <[email protected]> wrote:
> > > > I don't think we need syntax to describe it. As I just said in my
> > > > other reply, we have a perfectly good precedent for this already
> > > > in ordinary object permissions. That is: an object owner always,
> > > > implicitly, has GRANT OPTION for all the object's privileges, even
> > > > if she revoked the corresponding plain privilege from herself.
> > > >
> > > > Yeah, this does mean that we're effectively deciding that the creator
> > > > of a role is its owner. What's the problem with that?
> > >
> > > I don't think that's entirely the wrong concept, but it doesn't make a
> > > lot of sense in a world where the creator has to be a superuser. If
> > > alice, bob, and charlie are superusers who take turns creating new
> > > users, and then we let charlie go due to budget cuts, forcing alice
> > > and bob to change the owner of all the users he created to some other
> > > superuser as a condition of dropping his account is a waste of
> > > everyone's time. They can do exactly the same things to every account
> > > on the system after we change the role owner as before.
> >
> > Then maybe we should just implement the idea that if a superuser would
> > become the owner we instead substitute in the bootstrap user. Or give
> the
> > DBA the choice whether they want to retain knowledge of specific roles -
> > and thus are willing to accept the "waste of time".
>
> This doesn't strike me as going in the right direction. Falling back to
> the bootstrap superuser is generally a hack and not a great one. I'll
> also point out that the SQL spec hasn't got a concept of role ownership
> either.
>
> > > But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
> > > generally agreed to be broken right now, and if you don't agree with
> > > that, consider that it can grant pg_execute_server_programs to a
> > > newly-created account and then explain to me how it's functionally
> > > different from superuser.
> >
> > CREATEROLE has long been defined as basically having "with admin option"
> on
> > every role in the system. The failure to special-case the roles that
> grant
> > different aspects of superuser-ness to its members doesn't make
> CREATEROLE
> > itself broken, it makes the implementation of pg_execute_server_programs
> > broken. Only superusers should be considered to have with admin option
> on
> > these roles. They can delegate through the usual membership+admin
> mechanism
> > to a CREATEROLE role if they desire.
>
> No, CREATEROLE having admin option on every role in the system is broken
> and always has been. It's not just an issue for predefined roles like
> pg_execute_server_program,
> it's an issue for any role that could become
> a superuser either directly or indirectly and that extends beyond the
> predefined ones.
The only indirect way for a role to become superuser is to have been
granted membership in a superuser group, then SET ROLE. Non-superusers
cannot do this. If a superuser does this I consider the outcome to be no
different than if they go and do:
SET allow_system_table_mods TO true;
DROP pg_catalog.pg_class;
In short, having a CREATEROLE user issuing:
GRANT pg_read_all_stats TO davidj;
should result in the same outcome as them issuing:
GRANT postgres TO davidj;
-- ERROR: must be superuser to alter superusers
Superusers can break their system and we don't go to great effort to stop
them. I see no difference here, so arguments of this nature aren't all
that compelling to me.
CREATEROLE shouldn't be
> the determining factor in the question of if a role can GRANT a
> predefined (or any other role) to some other role- that should be
> governed by the admin option on that role, and that should work exactly
> the same for predefined roles as it does for any other.
>
Never granting the CREATEROLE attribute to anyone will give you this
outcome today.
> ADMIN option
> without membership isn't something the catalogs today can understand
>
Today, they don't need to in order for the system to function within its
existing design specs.
> > ALTER ROLE pg_superuser WITH [NO] ADMIN;
> >
> > Then adding a role membership including the WITH ADMIN OPTION can be
> > rejected, as can the non-superuser situation. Setting WITH NO ADMIN
> should
> > fail if any existing members have admin. You must be a superuser to
> > execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
> > new pg_* role that grants this ability (and maybe some others) for use
> by a
> > backup/restore user.
>
> I'm not following this in general or how it helps. Surely we don't want
> to limit WITH ADMIN to superusers.
Today a non-superuser cannot "grant postgres to someuser;"
The point of this attribute is to allow the superuser to apply that rule to
other roles that aren't superuser. In particular, the predefined pg_*
roles. But it could extend to any other role the superuser would like to
limit. It means, for that for named role, ADMIN privileges cannot be
delegated to other roles - thus all administration of that role's
membership roster must happen by a superuser.
In particular, this means CREATEROLE roles cannot assign membership in the
marked roles; just like they cannot assign membership in superuser roles
today.
For me, because the SUPERUSER cannot have its role become a group without a
superuser making that choice, and by default the default pg_* roles will
all have this property as well, and any newly superuser created roles that
may be members of either superuser or pg_* can have the property defined as
well, gives full control to the superuser as to how superuser abilities are
doled out and so the design itself allows for what many of you are
considering to be "safe usage". That "unsafe configurations" are possible
is due to the policy that superusers are unrestricted in what they can do,
including making unsafe and destructive choices.
In short, removing the self-administration rule solves the "login roles
should not be automatically considered groups administered by themselves"
problem - or at least a feature we really don't need.
And defining a "superuser administration only" attribute to a role solves
the indirect superuser privileges and assignment thereof by non-superusers
problem.
I can see value in adding a feature whereby we allow the DBA to define a
group as a schema-like container and then assign roles to that group with a
fine-grained permissions model. My take is this proposal is a new feature
while the two problems noted above can be solved more readily and with less
risk with the two suggested changes.
David J.
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 17:26 Joshua Brindle <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: Joshua Brindle @ 2022-03-10 17:26 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Stephen Frost <[email protected]>; David G. Johnston <[email protected]>; Tom Lane <[email protected]>; Mark Dilger <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Thu, Mar 10, 2022 at 12:11 PM Robert Haas <[email protected]> wrote:
>
> On Thu, Mar 10, 2022 at 11:19 AM Stephen Frost <[email protected]> wrote:
> > I disagree that ownership is needed that's not what the spec calls for
> > either. What we need is more flexibility when it comes to the
> > relationships which are allowed to be created between roles and what
> > privileges come with them. To that end, I'd argue that we should be
> > extending pg_auth_members, first by separating out membership itself
> > into an explicitly tracked attribute (instead of being implicit in the
> > existance of a row in the table) and then adding on what other
> > privileges we see fit to add, such as the ability to DROP a role. We
> > do need to remove the ability for a role who hasn't been explicitly
> > given the admin right on another role to modify that role's membership
> > too, as was originally proposed here. This also seems to more closely
> > follow the spec's expectation, something that role ownership doesn't.
>
> I do not have a problem with more fine-grained kinds of authorization
> even though I think there are syntactic issues to work out, but I
> strongly disagree with the idea that we can't or shouldn't also have
> role ownership. Marc invented it. Now Tom has invented it
> independently. All sorts of other objects have it already. Trying to
> make it out like this is some kind of kooky idea is not believable.
> Yeah, it's not the most sophisticated or elegant model and that's why
> it's good for us to also have other things, but for simple cases it is
> easy to understand and works great.
Ownership implies DAC, the ability to grant others rights to an
object. It's not "kooky" to see roles as owned objects, but it isn't
required either. For example most objects on a UNIX system are owned
and subject to DAC but users aren't.
Stephen's, and now my, issue with ownership is that, since it implies
DAC, most checks will be bypassed for the owner. We would both prefer
for everyone to be subject to the grants, including whoever created
the role.
Rather, we'd like to see a "creators of roles get this set of grants
against the role by default" and "as a superuser I can revoke grants
from creators against roles they created"
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 18:05 Stephen Frost <[email protected]>
parent: David G. Johnston <[email protected]>
0 siblings, 1 reply; 41+ messages in thread
From: Stephen Frost @ 2022-03-10 18:05 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Mark Dilger <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Greetings,
* David G. Johnston ([email protected]) wrote:
> On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <[email protected]> wrote:
> > * David G. Johnston ([email protected]) wrote:
> > > On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <[email protected]>
> > wrote:
> > > > On Wed, Mar 9, 2022 at 4:31 PM Tom Lane <[email protected]> wrote:
> > > > > I don't think we need syntax to describe it. As I just said in my
> > > > > other reply, we have a perfectly good precedent for this already
> > > > > in ordinary object permissions. That is: an object owner always,
> > > > > implicitly, has GRANT OPTION for all the object's privileges, even
> > > > > if she revoked the corresponding plain privilege from herself.
> > > > >
> > > > > Yeah, this does mean that we're effectively deciding that the creator
> > > > > of a role is its owner. What's the problem with that?
> > > >
> > > > I don't think that's entirely the wrong concept, but it doesn't make a
> > > > lot of sense in a world where the creator has to be a superuser. If
> > > > alice, bob, and charlie are superusers who take turns creating new
> > > > users, and then we let charlie go due to budget cuts, forcing alice
> > > > and bob to change the owner of all the users he created to some other
> > > > superuser as a condition of dropping his account is a waste of
> > > > everyone's time. They can do exactly the same things to every account
> > > > on the system after we change the role owner as before.
> > >
> > > Then maybe we should just implement the idea that if a superuser would
> > > become the owner we instead substitute in the bootstrap user. Or give
> > the
> > > DBA the choice whether they want to retain knowledge of specific roles -
> > > and thus are willing to accept the "waste of time".
> >
> > This doesn't strike me as going in the right direction. Falling back to
> > the bootstrap superuser is generally a hack and not a great one. I'll
> > also point out that the SQL spec hasn't got a concept of role ownership
> > either.
> >
> > > > But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
> > > > generally agreed to be broken right now, and if you don't agree with
> > > > that, consider that it can grant pg_execute_server_programs to a
> > > > newly-created account and then explain to me how it's functionally
> > > > different from superuser.
> > >
> > > CREATEROLE has long been defined as basically having "with admin option"
> > on
> > > every role in the system. The failure to special-case the roles that
> > grant
> > > different aspects of superuser-ness to its members doesn't make
> > CREATEROLE
> > > itself broken, it makes the implementation of pg_execute_server_programs
> > > broken. Only superusers should be considered to have with admin option
> > on
> > > these roles. They can delegate through the usual membership+admin
> > mechanism
> > > to a CREATEROLE role if they desire.
> >
> > No, CREATEROLE having admin option on every role in the system is broken
> > and always has been. It's not just an issue for predefined roles like
> > pg_execute_server_program,
>
>
>
> > it's an issue for any role that could become
> > a superuser either directly or indirectly and that extends beyond the
> > predefined ones.
>
>
> The only indirect way for a role to become superuser is to have been
> granted membership in a superuser group, then SET ROLE. Non-superusers
> cannot do this. If a superuser does this I consider the outcome to be no
> different than if they go and do:
A non-superuser absolutely can be GRANT'd membership in a superuser role
and then SET ROLE to that user thus becoming a superuser. Giving users
a regular role to log in as and then membership in a role that can
become a superuser is akin to having a sudoers group in Unix and is good
practice, not something that everyone should have to be super-dooper
careful to not do, lest a CREATEROLE user be able to leverage that.
> SET allow_system_table_mods TO true;
> DROP pg_catalog.pg_class;
I don't equate these in the least.
> In short, having a CREATEROLE user issuing:
> GRANT pg_read_all_stats TO davidj;
> should result in the same outcome as them issuing:
> GRANT postgres TO davidj;
> -- ERROR: must be superuser to alter superusers
No, what should matter is if the role doing the GRANT has admin rights
on pg_read_all_stats, or on the postgres role. That also happens to be
what the spec says.
> Superusers can break their system and we don't go to great effort to stop
> them. I see no difference here, so arguments of this nature aren't all
> that compelling to me.
That you don't feel they're compelling don't make them somehow not real,
nor even particularly uncommon, nor do I view ignoring that possibility
as somehow creating a strong authentication system.
> CREATEROLE shouldn't be
> > the determining factor in the question of if a role can GRANT a
> > predefined (or any other role) to some other role- that should be
> > governed by the admin option on that role, and that should work exactly
> > the same for predefined roles as it does for any other.
> >
>
> Never granting the CREATEROLE attribute to anyone will give you this
> outcome today.
... which is why CREATEROLE is broken.
> > ADMIN option
> > without membership isn't something the catalogs today can understand
>
> Today, they don't need to in order for the system to function within its
> existing design specs.
Eh? Your argument here is "don't use CREATEROLE"? While I agree with
that being a generally good idea today, it hardly makes sense to suggest
it in a thread where we're talking about how to make CREATEROLE, or
something like it, be useful.
> > > ALTER ROLE pg_superuser WITH [NO] ADMIN;
> > >
> > > Then adding a role membership including the WITH ADMIN OPTION can be
> > > rejected, as can the non-superuser situation. Setting WITH NO ADMIN
> > should
> > > fail if any existing members have admin. You must be a superuser to
> > > execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
> > > new pg_* role that grants this ability (and maybe some others) for use
> > by a
> > > backup/restore user.
> >
> > I'm not following this in general or how it helps. Surely we don't want
> > to limit WITH ADMIN to superusers.
>
> Today a non-superuser cannot "grant postgres to someuser;"
No, but a role can be created like 'admin', which a superuser GRANT's
'postgres' to and then that role can be GRANT'd to anyone by anyone who
has CREATEROLE rights. That's not sane.
> The point of this attribute is to allow the superuser to apply that rule to
> other roles that aren't superuser. In particular, the predefined pg_*
> roles. But it could extend to any other role the superuser would like to
> limit. It means, for that for named role, ADMIN privileges cannot be
> delegated to other roles - thus all administration of that role's
> membership roster must happen by a superuser.
The whole "X can't modify a superuser role without being a superuser"
concept is just broken and was a poor choice when it was originally
done specifically because it only looks at individual roles and their
specific rolsuper bit, completely ignoring the fact that role membership
exists as a thing that we should handle sanely, including a
non-superuser role being grant'd a superuser role. Predefined roles
haven't got anything to do with any of this, they only make it more
obvious to people who didn't understand how the system worked before
they came along.
I disagree entirely with the idea that we must have some roles who can
only ever be administered by a superuser. If anything, we should be
moving away (as we have, in fact, been doing), from anything being the
exclusive purview of the superuser.
> In particular, this means CREATEROLE roles cannot assign membership in the
> marked roles; just like they cannot assign membership in superuser roles
> today.
I disagree with the idea that we need to mark some roles as only being
able to be modified by the superuser- why invent this? We have the
ADMIN option already and that can be applied to allow any role X to have
the ability to modify the members of role Y. That's a whole lot better
than some explicit flag that says "only superusers can modify this
role". If an admin wants that, they can set things up that way already
today, as long as they don't use the current CREATEROLE attribute.
Ideally, we'd modify CREATEROLE, or remove it and replace it with
something better, which still maintains that same flexibility. What you
seem to be arguing for here is to rip out the ADMIN functionality, which
is defined by spec and not even exclusively by PG, and replace it with a
single per-role flag that says if that role can only be modified by
superusers. That seems entirely backwards to me.
> For me, because the SUPERUSER cannot have its role become a group without a
> superuser making that choice, and by default the default pg_* roles will
> all have this property as well, and any newly superuser created roles that
> may be members of either superuser or pg_* can have the property defined as
> well, gives full control to the superuser as to how superuser abilities are
> doled out and so the design itself allows for what many of you are
> considering to be "safe usage". That "unsafe configurations" are possible
> is due to the policy that superusers are unrestricted in what they can do,
> including making unsafe and destructive choices.
I disagree that it's an 'unsafe configuration' for there to ever exist a
non-superuser role that has been granted a superuser role. The only
thing that makes this unsafe is the existance of CREATEROLE.
Why are we making this all about superusers though? In what you're
proposing, you're suggesting that it's perfectly fine for any role which
has CREATEROLE to be able to take over any other role in the entire
system, excluding predefined roles and superusers. How is that sane, or
truely much less than what the superuser has in terms of ability? The
short answer is that it's not- which is why we have documented
CREATEROLE as being 'superuser light'. The goal here is to get rid of
that.
> In short, removing the self-administration rule solves the "login roles
> should not be automatically considered groups administered by themselves"
> problem - or at least a feature we really don't need.
> And defining a "superuser administration only" attribute to a role solves
> the indirect superuser privileges and assignment thereof by non-superusers
> problem.
But it doesn't *actually* make CREATEROLE something that you can give
out to folks on a general basis because anyone with CREATEROLE would
still be able to take over every single non-superuser and non-predefined
role in the system. We do *not* want that.
> I can see value in adding a feature whereby we allow the DBA to define a
> group as a schema-like container and then assign roles to that group with a
> fine-grained permissions model. My take is this proposal is a new feature
> while the two problems noted above can be solved more readily and with less
> risk with the two suggested changes.
Yes, we're talking about a new feature- one intended to replace the
broken way that CREATEROLE works, which your proposal doesn't.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 41+ messages in thread
* Re: role self-revocation
@ 2022-03-10 19:31 David G. Johnston <[email protected]>
parent: Stephen Frost <[email protected]>
0 siblings, 0 replies; 41+ messages in thread
From: David G. Johnston @ 2022-03-10 19:31 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Mark Dilger <[email protected]>; Joshua Brindle <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Thu, Mar 10, 2022 at 11:05 AM Stephen Frost <[email protected]> wrote:
> Greetings,
>
> * David G. Johnston ([email protected]) wrote:
> > On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <[email protected]>
> wrote:
> > > * David G. Johnston ([email protected]) wrote:
> > > > On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <[email protected]>
> > > wrote:
>
> > The only indirect way for a role to become superuser is to have been
> > granted membership in a superuser group, then SET ROLE. Non-superusers
> > cannot do this. If a superuser does this I consider the outcome to be no
> > different than if they go and do:
>
> A non-superuser absolutely can be GRANT'd membership in a superuser role
> and then SET ROLE to that user thus becoming a superuser.
A non-superuser cannot grant a non-superuser membership in a superuser
role. A superuser granting a user membership in a superuser role makes
that user a superuser. This seems sane.
If a superuser grants a non-superuser membership in a superuser role then
today a non-superuser can grant a user membership in that intermediate
role, thus having a non-superuser make another user a superuser. This is
arguably a bug that needs to be fixed.
My desired fix is to just require the superuser to mark (or have it marked
by default ideally) the role inheriting superuser and put the
responsibility on the superuser. I agree this is not ideal, but it is
probably quick and low risk.
I'll let someone else describe the details of the alternative option. I
suspect it will end up being a better option in terms of design. But
depending on time and risk even knowing that we want the better design
eventually doesn't preclude getting the easier fix in now.
> No, what should matter is if the role doing the GRANT has admin rights
> on pg_read_all_stats, or on the postgres role. That also happens to be
> what the spec says.
>
Yes, and superusers implicitly have that right, while CREATEROLE users
implicitly have that right on the pg_* role but not on superuser roles. I
just want to plug that hole and include the pg_* roles (or any role for
that matter) in being able to be denied implied ADMIN rights for
non-superusers.
> Today a non-superuser cannot "grant postgres to someuser;"
>
> No, but a role can be created like 'admin', which a superuser GRANT's
> 'postgres' to and then that role can be GRANT'd to anyone by anyone who
> has CREATEROLE rights. That's not sane.
>
I agree. And I've suggested a minimal fix, adding an attribute to the role
that prohibits non-superusers from granting it to others, that removes the
insane behavior.
I'm on board for a hard-coded fix as well - if a superuser is in the
membership chain of a role then non-superusers cannot grant membership in
that role to others.
Neither of those really solves the pg_* roles problem. We still need to
indicate that they are somehow special. Whether it is a nice matrix or
roles and permissions or a simple attribute that makes them behave like
they are superuser roles.
>
> I disagree entirely with the idea that we must have some roles who can
> only ever be administered by a superuser.
I don't think this is a must have. I think that since we do have it today
that fixes that leverage the status quo in order to be done more easily are
perfectly valid solutions.
> If anything, we should be
> moving away (as we have, in fact, been doing), from anything being the
> exclusive purview of the superuser.
>
I totally agree.
>
> > In particular, this means CREATEROLE roles cannot assign membership in
> the
> > marked roles; just like they cannot assign membership in superuser roles
> > today.
>
> I disagree with the idea that we need to mark some roles as only being
> able to be modified by the superuser- why invent this?
Because CREATEUSER is a thing and people want to prevent roles with that
attribute from assigning membership to the predefined superuser-aspect
roles. If I've misunderstood that desire and the scope of delegation given
by the superuser to CREATEUSER roles is acceptable, then no change here is
needed.
What you
> seem to be arguing for here is to rip out the ADMIN functionality, which
> is defined by spec and not even exclusively by PG, and replace it with a
> single per-role flag that says if that role can only be modified by
> superusers.
I made the observation that being able to manage the membership of a group
without having the ability to create new users seems like a half a loaf of
a feature. That's it. I would presume that any redesign of the
permissions system here would address this adequately.
The
>
short answer is that it's not- which is why we have documented
> CREATEROLE as being 'superuser light'. The goal here is to get rid of
> that.
>
Now you tell me. Robert should have led with that goal upfront.
>
> Yes, we're talking about a new feature- one intended to replace the
> broken way that CREATEROLE works, which your proposal doesn't.
>
>
That is correct, I was trying to figure out minimally invasive fixes to
what are arguably being called bugs.
David J.
^ permalink raw reply [nested|flat] 41+ messages in thread
end of thread, other threads:[~2022-03-10 19:31 UTC | newest]
Thread overview: 41+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-12-13 06:07 [PATCH v10 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v13 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v11 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v20 3/4] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v7 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v17 3/4] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v6 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v14 3/3] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v18 3/4] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v9 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v16 3/4] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v12 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v10 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v8 6/6] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v5 6/8] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v4 6/8] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v15 3/4] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2021-12-13 06:07 [PATCH v19 3/4] Move removal of old logical rewrite mapping files to custodian. Nathan Bossart <[email protected]>
2022-03-07 20:03 Re: role self-revocation Mark Dilger <[email protected]>
2022-03-07 20:09 ` Re: role self-revocation Mark Dilger <[email protected]>
2022-03-07 20:16 ` Re: role self-revocation Tom Lane <[email protected]>
2022-03-07 21:34 ` Re: role self-revocation David G. Johnston <[email protected]>
2022-03-08 04:14 ` Re: role self-revocation Mark Dilger <[email protected]>
2022-03-09 20:51 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-09 21:01 ` Re: role self-revocation Tom Lane <[email protected]>
2022-03-09 21:15 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-09 21:23 ` Re: role self-revocation Stephen Frost <[email protected]>
2022-03-09 21:31 ` Re: role self-revocation Tom Lane <[email protected]>
2022-03-09 22:00 ` Re: role self-revocation David G. Johnston <[email protected]>
2022-03-09 22:35 ` Re: role self-revocation Tom Lane <[email protected]>
2022-03-10 14:46 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-10 15:56 ` Re: role self-revocation David G. Johnston <[email protected]>
2022-03-10 16:19 ` Re: role self-revocation Stephen Frost <[email protected]>
2022-03-10 17:11 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-10 17:26 ` Re: role self-revocation Joshua Brindle <[email protected]>
2022-03-10 17:26 ` Re: role self-revocation David G. Johnston <[email protected]>
2022-03-10 18:05 ` Re: role self-revocation Stephen Frost <[email protected]>
2022-03-10 19:31 ` Re: role self-revocation David G. Johnston <[email protected]>
2022-03-10 16:26 ` Re: role self-revocation Mark Dilger <[email protected]>
2022-03-09 21:20 ` Re: role self-revocation Stephen Frost <[email protected]>
2022-03-09 21:24 ` Re: role self-revocation Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox