[PATCH 2/2] Add monitoring aid for max_replication

agora inbox for [email protected]  
help / color / mirror / Atom feed

[PATCH 2/2] Add monitoring aid for max_replication_slots.
19+ messages / 4 participants
[nested] [flat]

* [PATCH 2/2] Add monitoring aid for max_replication_slots.
@ 2017-09-07 10:13  Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Kyotaro Horiguchi @ 2017-09-07 10:13 UTC (permalink / raw)

Adds two columns "live" and "distance" in pg_replication_slot.
Setting max_slot_wal_keep_size, long-disconnected slots may lose sync.
The two columns shows how long a slot can live on or how many bytes a
slot have lost if max_slot_wal_keep_size is set.
---
 src/backend/access/transam/xlog.c    | 128 ++++++++++++++++++++++++++++++++++-
 src/backend/catalog/system_views.sql |   4 +-
 src/backend/replication/slotfuncs.c  |  25 ++++++-
 src/include/access/xlog.h            |   1 +
 src/include/catalog/pg_proc.h        |   2 +-
 src/test/regress/expected/rules.out  |   6 +-
 6 files changed, 160 insertions(+), 6 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index cfdae39..be53e0f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9402,6 +9402,128 @@ CreateRestartPoint(int flags)
 	return true;
 }
 
+
+/*
+ * Returns the segment number of the oldest file in XLOG directory.
+ */
+static XLogSegNo
+GetOldestXLogFileSegNo(void)
+{
+	DIR		*xldir;
+	struct dirent *xlde;
+	XLogSegNo segno = 0;
+
+	xldir = AllocateDir(XLOGDIR);
+	if (xldir == NULL)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not open write-ahead log directory \"%s\": %m",
+						XLOGDIR)));
+
+	while ((xlde = ReadDir(xldir, XLOGDIR)) != NULL)
+	{
+		TimeLineID tli;
+		XLogSegNo fsegno;
+
+		/* Ignore files that are not XLOG segments */
+		if (!IsXLogFileName(xlde->d_name) &&
+			!IsPartialXLogFileName(xlde->d_name))
+			continue;
+
+		XLogFromFileName(xlde->d_name, &tli, &fsegno, wal_segment_size);
+
+		/* get minimum segment ignorig timeline ID */
+		if (segno == 0 || fsegno < segno)
+			segno = fsegno;
+	}
+
+	return segno;
+}
+
+/*
+ * Check if the record on the given restartLSN is present in XLOG files.
+ *
+ * Returns true if it is present. If minSecureLSN is given, it receives the
+ * LSN at the beginning of the oldest existing WAL segment.
+ */
+bool
+IsLsnStillAvaiable(XLogRecPtr restartLSN, XLogRecPtr *minSecureLSN)
+{
+	XLogRecPtr currpos;
+	XLogSegNo currSeg;
+	XLogSegNo restartSeg;
+	XLogSegNo tailSeg;
+	XLogSegNo oldestSeg;
+	uint64 keepSegs;
+
+	currpos = GetXLogWriteRecPtr();
+
+	SpinLockAcquire(&XLogCtl->info_lck);
+	oldestSeg = XLogCtl->lastRemovedSegNo;
+	SpinLockRelease(&XLogCtl->info_lck);
+
+	/*
+	 * oldestSeg is zero before at least one segment has been removed since
+	 * startup. Use oldest segno taken from file names.
+	 */
+	if (oldestSeg == 0)
+	{
+		static XLogSegNo oldestFileSeg = 0;
+
+		if (oldestFileSeg == 0)
+			oldestFileSeg = GetOldestXLogFileSegNo();
+		/* let it have the same meaning with lastRemovedSegNo here */
+		oldestSeg = oldestFileSeg - 1;
+	}
+
+	/* oldest segment is just after the last removed segment */
+	oldestSeg++;
+
+	XLByteToSeg(currpos, currSeg, wal_segment_size);
+	XLByteToSeg(restartLSN, restartSeg, wal_segment_size);
+
+
+	if (minSecureLSN)
+	{
+		if (max_slot_wal_keep_size_mb > 0)
+		{
+			uint64 slotlimitbytes = 1024 * 1024 * max_slot_wal_keep_size_mb;
+			uint64 slotlimitfragment = XLogSegmentOffset(slotlimitbytes,
+														 wal_segment_size);
+			uint64 currposoff = XLogSegmentOffset(currpos, wal_segment_size);
+
+			/* Calculate keep segments. Must be in sync with KeepLogSeg. */
+			Assert(wal_keep_segments >= 0);
+			Assert(max_slot_wal_keep_size_mb >= 0);
+
+			keepSegs = wal_keep_segments +
+				ConvertToXSegs(max_slot_wal_keep_size_mb, wal_segment_size);
+			if (currposoff < slotlimitfragment)
+				keepSegs++;
+
+			/*
+			 * calculate the oldest segment that will be kept by
+			 * wal_keep_segments and max_slot_wal_keep_size_mb
+			 */
+			if (currSeg < keepSegs)
+				tailSeg = 0;
+			else
+				tailSeg = currSeg - keepSegs;
+
+		}
+		else
+		{
+			/* all requred segments are secured in this case */
+			XLogRecPtr keep = XLogGetReplicationSlotMinimumLSN();
+			XLByteToSeg(keep, tailSeg, wal_segment_size);
+		}
+
+		XLogSegNoOffsetToRecPtr(tailSeg, 0, *minSecureLSN, wal_segment_size);
+	}
+
+	return	oldestSeg <= restartSeg;
+}
+
 /*
  * Retreat *logSegNo to the last segment that we need to retain because of
  * either wal_keep_segments or replication slots.
@@ -9429,7 +9551,11 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
 			segno = segno - wal_keep_segments;
 	}
 
-	/* then check whether slots limit removal further */
+	/*
+	 * then check whether slots limit removal further
+	 * should be consistent with IsLsnStillAvaiable().
+	 */
+
 	if (max_replication_slots > 0 && keep != InvalidXLogRecPtr)
 	{
 		XLogSegNo	slotSegNo;
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index dc40cde..6512ac3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -793,7 +793,9 @@ CREATE VIEW pg_replication_slots AS
             L.xmin,
             L.catalog_xmin,
             L.restart_lsn,
-            L.confirmed_flush_lsn
+            L.confirmed_flush_lsn,
+			L.status,
+			L.min_secure_lsn
     FROM pg_get_replication_slots() AS L
             LEFT JOIN pg_database D ON (L.datoid = D.oid);
 
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index ab776e8..200a478 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -182,7 +182,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 Datum
 pg_get_replication_slots(PG_FUNCTION_ARGS)
 {
-#define PG_GET_REPLICATION_SLOTS_COLS 11
+#define PG_GET_REPLICATION_SLOTS_COLS 13
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
 	TupleDesc	tupdesc;
 	Tuplestorestate *tupstore;
@@ -304,6 +304,29 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 		else
 			nulls[i++] = true;
 
+		if (restart_lsn == InvalidXLogRecPtr)
+		{
+			values[i++] = CStringGetTextDatum("unknown");
+			values[i++] = LSNGetDatum(InvalidXLogRecPtr);
+		}
+		else
+		{
+			XLogRecPtr	min_secure_lsn;
+			char *status = "borken";
+
+			if (BoolGetDatum(IsLsnStillAvaiable(restart_lsn,
+												&min_secure_lsn)))
+			{
+				if (min_secure_lsn <= restart_lsn)
+					status = "secured";
+				else
+					status = "insecured";
+			}
+
+			values[i++] = CStringGetTextDatum(status);
+			values[i++] = LSNGetDatum(min_secure_lsn);
+		}
+
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
 	LWLockRelease(ReplicationSlotControlLock);
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index f0c0255..a316ead 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -269,6 +269,7 @@ extern void ShutdownXLOG(int code, Datum arg);
 extern void InitXLOGAccess(void);
 extern void CreateCheckPoint(int flags);
 extern bool CreateRestartPoint(int flags);
+extern bool IsLsnStillAvaiable(XLogRecPtr restartLSN, XLogRecPtr *minSecureLSN);
 extern void XLogPutNextOid(Oid nextOid);
 extern XLogRecPtr XLogRestorePoint(const char *rpName);
 extern void UpdateFullPageWrites(void);
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 93c031a..d03fd6f 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5340,7 +5340,7 @@ DATA(insert OID = 3779 (  pg_create_physical_replication_slot PGNSP PGUID 12 1 0
 DESCR("create a physical replication slot");
 DATA(insert OID = 3780 (  pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 1 0 2278 "19" _null_ _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
 DESCR("drop a replication slot");
-DATA(insert OID = 3781 (  pg_get_replication_slots	PGNSP PGUID 12 1 10 0 0 f f f f f t s s 0 0 2249 "" "{19,19,25,26,16,16,23,28,28,3220,3220}" "{o,o,o,o,o,o,o,o,o,o,o}" "{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn}" _null_ _null_ pg_get_replication_slots _null_ _null_ _null_ ));
+DATA(insert OID = 3781 (  pg_get_replication_slots	PGNSP PGUID 12 1 10 0 0 f f f f f t s s 0 0 2249 "" "{19,19,25,26,16,16,23,28,28,3220,3220,25,3220}" "{o,o,o,o,o,o,o,o,o,o,o,o,o}" "{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,status,min_secure_lsn}" _null_ _null_ pg_get_replication_slots _null_ _null_ _null_ ));
 DESCR("information about replication slots currently in use");
 DATA(insert OID = 3786 (  pg_create_logical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 3 0 2249 "19 19 16" "{19,19,16,25,3220}" "{i,i,i,o,o}" "{slot_name,plugin,temporary,slot_name,lsn}" _null_ _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));
 DESCR("set up a logical replication slot");
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index f1c1b44..d9d74a3 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1451,8 +1451,10 @@ pg_replication_slots| SELECT l.slot_name,
     l.xmin,
     l.catalog_xmin,
     l.restart_lsn,
-    l.confirmed_flush_lsn
-   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn)
+    l.confirmed_flush_lsn,
+    l.status,
+    l.min_secure_lsn
+   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, status, min_secure_lsn)
      LEFT JOIN pg_database d ON ((l.datoid = d.oid)));
 pg_roles| SELECT pg_authid.rolname,
     pg_authid.rolsuper,
-- 
2.9.2


----Next_Part(Thu_Nov_09_17_31_28_2017_905)--
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

----Next_Part(Thu_Nov_09_17_31_28_2017_905)----





^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 17/23] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email protected]

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index a1541db2bc8..e5e5d113c4c 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -28,6 +28,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -185,6 +192,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -204,6 +212,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -227,6 +236,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--tKkaNMvYmhQvRCRK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0018-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 20/25] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email protected]

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index d8f46a69296..f66ce2d7146 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -28,6 +28,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -185,6 +192,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -204,6 +212,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -227,6 +236,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--IS0zKkzwUGydFO0o
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0021-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 16/19] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index 52092f83bc6..89de2753f9f 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -26,6 +26,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -177,6 +184,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -196,6 +204,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -215,6 +224,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--Sw7tCqrGA+HQ0/zt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0017-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 17/21] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email protected]

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index d37761c6be2..f9102e4ea1c 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -28,6 +28,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -183,6 +190,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -202,6 +210,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -225,6 +234,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--STPqjqpCrtky8aYs
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0018-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 17/23] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email protected]

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index a1541db2bc8..e5e5d113c4c 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -28,6 +28,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -185,6 +192,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -204,6 +212,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -227,6 +236,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--tKkaNMvYmhQvRCRK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0018-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 4/7] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

ci-os-only: linux
---
 .cirrus.yml                       | 20 ++++++++++++++++++--
 src/tools/ci/code-coverage-report | 25 +++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index a3af4a0a808..a5c3b97925f 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -26,6 +26,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patches series manually submitted to cirrus may benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -175,6 +182,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -187,13 +195,14 @@ task:
     chown root:postgres /tmp/cores
     sysctl kernel.core_pattern='/tmp/cores/%e-%s-%p.core'
   setup_additional_packages_script: |
-    #apt-get update
-    #apt-get -y install ...
+    apt-get update
+    apt-get -y install lcov
 
   configure_script: |
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -213,6 +222,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..57126247ea0
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,25 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'`
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--wLAMOaPNJ0fu1fTG
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0005-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 16/19] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index 52092f83bc6..89de2753f9f 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -26,6 +26,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -177,6 +184,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -196,6 +204,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -215,6 +224,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--Sw7tCqrGA+HQ0/zt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0017-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 16/19] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index 52092f83bc6..89de2753f9f 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -26,6 +26,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -177,6 +184,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -196,6 +204,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -215,6 +224,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--Sw7tCqrGA+HQ0/zt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0017-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 7/9] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

Some alternatives:
- could build with "--coverage -fprofile-filter-files=", but that means
  that ccache will never work (both because it doesn't support that option
  and also because the arguments will be different for every patch).
- could use ninja coverage-html, but that can't filter only changed
  files, and would take a long time to upload a lot of useless files.

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email protected]

ci-os-only: freebsd
---
 .cirrus.yml                       | 18 +++++++++++++++-
 src/tools/ci/code-coverage-report | 35 +++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index a9fa4b5af27..ec15263a665 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -28,6 +28,14 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  #BASE_COMMIT: HEAD~1
+  # For demo purposes:
+  BASE_COMMIT: HEAD~11
 
 # What files to preserve in case tests fail
 on_failure_ac: &on_failure_ac
@@ -157,6 +165,7 @@ task:
     uname -a
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
 
   ccache_cache:
     folder: $CCACHE_DIR
@@ -170,7 +179,7 @@ task:
     chown root:postgres /tmp/cores
     sysctl kern.corefile='/tmp/cores/%N.%P.core'
   setup_additional_packages_script: |
-    #pkg install -y ...
+    pkg install -y lcov
 
   # NB: Intentionally build without -Dllvm. The freebsd image size is already
   # large enough to make VM startup slow
@@ -178,6 +187,7 @@ task:
     su postgres <<-EOF
       meson setup \
         --buildtype=debug \
+        -Db_coverage=true \
         -Dcassert=true -Dssl=openssl -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
         -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
         -Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
@@ -188,10 +198,16 @@ task:
 
   test_world_script: |
     su postgres <<-EOF
+      set -e
       ulimit -c unlimited
       meson test $MTEST_ARGS --num-processes ${TEST_JOBS}
+      # Create coverage report for files changed since the base commit.
+      time ./src/tools/ci/code-coverage-report "$BASE_COMMIT" ./build ./coverage
     EOF
 
+  coverage_artifacts:
+    path: 'coverage/**'
+
   # test runningcheck, freebsd chosen because it's currently fast enough
   test_running_script: |
     su postgres <<-EOF
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..4f29d3f17a7
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,35 @@
+#! /bin/sh
+# Called during CI to generate a code coverage report of changed files.
+set -e
+
+base_branch=$1
+build_dir=$2
+outdir=$3
+
+changed=`git diff --name-only "$base_branch" '*.c'`
+[ -z "$changed" ] && exit 0 # Nothing changed
+
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+# This could be used to map from object file back to source file:
+# readelf --debug-dump=info src/backend/postgres_lib.a.p/utils_adt_array_userfuncs.c.o |awk '/DW_AT_name/{print $NF;exit}'
+# gcov ./src/port/libpgport_shlib.a.p/inet_net_ntop.c.gcno --stdout
+
+gcov=$outdir/coverage.gcov
+lcov --quiet --capture --directory "$build_dir" >"$gcov.all"
+
+# Filter to include only changed files
+echo "$changed" |sed 's,^,*/,' |
+	xargs -rt lcov --extract "$gcov.all" >"$gcov"
+
+ls -l "$outdir"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.25.1


--61jdw2sOBCFtR2d/
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0008-cirrus-upload-changed-html-docs-as-artifacts.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 6/8] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

Coverage is shown only for changed files.  This is useful to see
coverage of newly-added code, but won't show added/lost coverage in
files which this patch doesn't modify.

Some alternatives:
- could build with "--coverage -fprofile-filter-files=", but that means
  that ccache will never work (both because it doesn't support that option
  and also because the arguments will be different for every patch).
- could use ninja coverage-html, but that can't filter only changed
  files, and would take a long time and a lot of space to upload a lot
  of useless files.

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email protected]

ci-os-only: freebsd
---
 .cirrus.yml                       | 20 ++++++++++++-
 src/tools/ci/code-coverage-report | 48 +++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index 6cbbea2f1e1..cea8b6fef2b 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -20,22 +20,30 @@ env:
 
   # target to test, for all but windows
   CHECK: check-world PROVE_FLAGS=$PROVE_FLAGS
   CHECKFLAGS: -Otarget
   PROVE_FLAGS: --timer
   MTEST_ARGS: --print-errorlogs --no-rebuild -C build
   PG_FAILED_TESTDIR: ${CIRRUS_WORKING_DIR}/failed.build
   PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  #BASE_COMMIT: HEAD~1
+  # For demo purposes:
+  BASE_COMMIT: HEAD~11
 
 # What files to preserve in case tests fail
 on_failure_ac: &on_failure_ac
   log_artifacts:
     paths:
       - "**/*.log"
       - "**/*.diffs"
       - "**/regress_log_*"
     type: text/plain
 
 on_failure_meson: &on_failure_meson
@@ -149,57 +157,67 @@ task:
     image: family/pg-ci-freebsd-13
     platform: freebsd
     cpu: $CPUS
     memory: 4G
     disk: 50
 
   sysinfo_script: |
     id
     uname -a
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
 
   ccache_cache:
     folder: $CCACHE_DIR
   create_user_script: |
     pw useradd postgres
     chown -R postgres:postgres .
     mkdir -p ${CCACHE_DIR}
     chown -R postgres:postgres ${CCACHE_DIR}
   setup_core_files_script: |
     mkdir -m 770 /tmp/cores
     chown root:postgres /tmp/cores
     sysctl kern.corefile='/tmp/cores/%N.%P.core'
   setup_additional_packages_script: |
-    #pkg install -y ...
+    pkg install -y lcov
 
   # NB: Intentionally build without -Dllvm. The freebsd image size is already
   # large enough to make VM startup slow
   configure_script: |
     su postgres <<-EOF
       meson setup \
         --buildtype=debug \
+        -Db_coverage=true \
         -Dcassert=true -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
         -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
         -Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
         build
     EOF
   build_script: su postgres -c 'ninja -C build -j${BUILD_JOBS}'
   upload_caches: ccache
 
   test_world_script: |
     su postgres <<-EOF
+      set -e
       ulimit -c unlimited
+      # Write initial coverage files before running tests:
+      time ./src/tools/ci/code-coverage-report "$BASE_COMMIT" ./build ./coverage "--initial"
       meson test $MTEST_ARGS --num-processes ${TEST_JOBS}
+      # Create coverage report for files changed since the base commit.
+      time ./src/tools/ci/code-coverage-report "$BASE_COMMIT" ./build ./coverage
     EOF
 
+  coverage_artifacts:
+    path: 'coverage/**'
+
   # test runningcheck, freebsd chosen because it's currently fast enough
   test_running_script: |
     su postgres <<-EOF
       set -e
       ulimit -c unlimited
       meson test $MTEST_ARGS --quiet --suite setup
       export LD_LIBRARY_PATH="$(pwd)/build/tmp_install/usr/local/pgsql/lib/:$LD_LIBRARY_PATH"
       mkdir -p build/testrun ${PG_FAILED_TESTDIR}
       build/tmp_install/usr/local/pgsql/bin/initdb -N build/runningcheck --no-instructions -A trust
       echo "include '$(pwd)/src/tools/ci/pg_ci_base.conf'" >> build/runningcheck/postgresql.conf
       build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l ${PG_FAILED_TESTDIR}/runningcheck.log start
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..db448e802ba
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,48 @@
+#! /bin/sh
+# Called during CI to generate a code coverage report of changed files.
+set -e
+
+base_branch=$1
+build_dir=$2
+outdir=$3
+args=$4
+
+changed=`git diff --name-only "$base_branch" '*.c'`
+[ -z "$changed" ] && exit 0 # Nothing changed
+
+[ -d "$outdir" ] ||
+	mkdir "$outdir"
+
+# This could be used to map from object file back to source file:
+# readelf --debug-dump=info src/backend/postgres_lib.a.p/utils_adt_array_userfuncs.c.o |awk '/DW_AT_name/{print $NF;exit}'
+# gcov ./src/port/libpgport_shlib.a.p/inet_net_ntop.c.gcno --stdout
+
+gcov=$outdir/coverage.gcov
+lcov --quiet --capture --directory "$build_dir" $args >"$gcov.new"
+
+# Filter to include only changed files
+echo "$changed" |sed 's,^,*/,' |
+	xargs -rt lcov --extract "$gcov.new" >"$gcov.filtered"
+rm "$gcov.new"
+
+echo "$args" |grep initial >/dev/null && {
+	mv "$gcov.filtered" "$gcov.init"
+	exit 0
+}
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov.filtered" ] || {
+	rm "$gcov.init"
+	exit 0
+}
+
+# Otherwise combine with the init file:
+lcov -a "$gcov.init" -a "$gcov.filtered" >"$gcov"
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" \
+	--title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
+
+gzip "$gcov" "$gcov.init" "$gcov.filtered"
+ls -l "$outdir"
+du -sh "$outdir"
-- 
2.34.1


--Wo0n/IWMQc9EwEbt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0007-cirrus-upload-changed-html-docs-as-artifacts.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* [PATCH 17/21] cirrus: code coverage
@ 2022-01-17 06:54  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email protected]

ci-os-only: linux
---
 .cirrus.yml                       | 16 ++++++++++++++++
 src/tools/ci/code-coverage-report | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index 2e1290bc4a4..e09dbfe3857 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -28,6 +28,13 @@ env:
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  BASE_COMMIT: HEAD~1
+
 
 # What files to preserve in case tests fail
 on_failure: &on_failure
@@ -183,6 +190,7 @@ task:
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
   create_user_script: |
     useradd -m postgres
     chown -R postgres:postgres .
@@ -202,6 +210,7 @@ task:
     su postgres <<-EOF
       ./configure \
         --enable-cassert --enable-debug --enable-tap-tests \
+        --enable-coverage \
         --enable-nls \
         \
         ${LINUX_CONFIGURE_FEATURES} \
@@ -225,6 +234,13 @@ task:
       make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
     EOF
 
+  # Build coverage report for files changed since the base commit.
+  generate_coverage_report_script: |
+    src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+  coverage_artifacts:
+    paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
   on_failure:
     <<: *on_failure
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..0dce149dcf8
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'` ||
+	[ $? -eq 1 ]
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
-- 
2.17.1


--mln0rGgUGuXEqmuI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0018-cirrus-build-docs-as-a-separate-task.patch"



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Preventing non-superusers from altering session authorization
@ 2023-07-09 17:03  Joseph Koshakow <[email protected]>
  0 siblings, 1 reply; 19+ messages in thread

From: Joseph Koshakow @ 2023-07-09 17:03 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Sun, Jul 9, 2023 at 12:47 AM Nathan Bossart <[email protected]>
wrote:

> I think we should split this into two patches: one to move the permission
> check to check_session_authorization() and another for the behavior
change.
> I've attached an attempt at the first one (that borrows heavily from your
> latest patch).  AFAICT the only reason that the permission check lives in
> SetSessionAuthorization() is because AuthenticatedUserIsSuperuser is
static
> to miscinit.c and doesn't have an accessor function.  I added one, but it
> would probably just be removed by the following patch.  WDYT?

I think that's a good idea. We could even keep around the accessor
function as a good place to bundle the calls to
    Assert(OidIsValid(AuthenticatedUserId))
and
    superuser_arg(AuthenticatedUserId)

> * Only a superuser may set auth ID to something other than himself

Is "auth ID" the right term here? Maybe something like "Only a
superuser may set their session authorization/ID to something other
than their authenticated ID."

>   But we set the GUC variable
> * is_superuser to indicate whether the *current* session userid is a
> * superuser.

Just a small correction here, I believe the is_superuser GUC is meant
to indicate whether the current user id is a superuser, not the current
session user id. We only update is_superuser in SetSessionAuthorization
because we are also updating the current user id in SetSessionUserId.
For example,

    test=# CREATE ROLE r1 SUPERUSER;
    CREATE ROLE
    test=# CREATE ROLE r2;
    CREATE ROLE
    test=# SET SESSION AUTHORIZATION r1;
    SET
    test=# SET ROLE r2;
    SET
    test=> SELECT session_user, current_user;
     session_user | current_user
    --------------+--------------
     r1           | r2
    (1 row)

    test=> SHOW is_superuser;
     is_superuser
    --------------
     off
    (1 row)

Which has also made me realize that the comment on is_superuser in
guc_tables.c is incorrect:

> /* Not for general use --- used by SET SESSION AUTHORIZATION */

Additionally the C variable name for is_superuser is fairly misleading:

> session_auth_is_superuser

The documentation for this GUC in show.sgml is correct:

> True if the current role has superuser privileges.

As an aside, I'm starting to think we should consider removing this
GUC. It sometimes reports an incorrect value [0], and potentially is
not used internally for anything.

I've rebased my changes over your patch and attached them both.

[0]
https://www.postgresql.org/message-id/CAAvxfHcxH-hLndty6CRThGXL1hLsgCn%2BE3QuG_4Qi7GxrHmgKg%40mail.g...


Attachments:

  [text/x-patch] v5-0002-Prevent-non-superusers-from-altering-session-auth.patch (4.9K, ../../CAAvxfHez_S2VpgdXtHe5oYt=cUREFBctdpmd2iVmLHVmJBsTmg@mail.gmail.com/3-v5-0002-Prevent-non-superusers-from-altering-session-auth.patch)
  download | inline diff:
From 2e1689b5fe384d675043beb9df8eff49a0ff436e Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email protected]>
Date: Sun, 9 Jul 2023 12:58:41 -0400
Subject: [PATCH 2/2] Prevent non-superusers from altering session auth

Previously, if a user connected with as a role that had the superuser
attribute, then they could always execute a SET SESSION AUTHORIZATION
statement for the duration of their session. Even if the role was
altered to set superuser to false, the user was still allowed to
execute SET SESSION AUTHORIZATION. This allowed them to set their
session role to some other superuser and effectively regain the
superuser privileges. They could even reset their own superuser
attribute to true.

This commit alters the privilege checks for SET SESSION AUTHORIZATION
such that a user can only execute it if the role they connected with is
currently a superuser. This prevents users from regaining superuser
privileges after it has been revoked.
---
 doc/src/sgml/ref/set_session_auth.sgml |  2 +-
 src/backend/utils/init/miscinit.c      | 19 +++++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml
index f8fcafc194..94adab2468 100644
--- a/doc/src/sgml/ref/set_session_auth.sgml
+++ b/doc/src/sgml/ref/set_session_auth.sgml
@@ -51,7 +51,7 @@ RESET SESSION AUTHORIZATION
 
   <para>
    The session user identifier can be changed only if the initial session
-   user (the <firstterm>authenticated user</firstterm>) had the
+   user (the <firstterm>authenticated user</firstterm>) has the
    superuser privilege.  Otherwise, the command is accepted only if it
    specifies the authenticated user name.
   </para>
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index f5548a0f47..1aa393f9fd 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -467,7 +467,7 @@ ChangeToDataDir(void)
  * AuthenticatedUserId is determined at connection start and never changes.
  *
  * SessionUserId is initially the same as AuthenticatedUserId, but can be
- * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserIsSuperuser).
+ * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserId is a superuser).
  * This is the ID reported by the SESSION_USER SQL function.
  *
  * OuterUserId is the current user ID in effect at the "outer level" (outside
@@ -492,8 +492,7 @@ static Oid	OuterUserId = InvalidOid;
 static Oid	CurrentUserId = InvalidOid;
 static const char *SystemUser = NULL;
 
-/* We also have to remember the superuser state of some of these levels */
-static bool AuthenticatedUserIsSuperuser = false;
+/* We also have to remember the superuser state of some of the session user */
 static bool SessionUserIsSuperuser = false;
 
 static int	SecurityRestrictionContext = 0;
@@ -583,13 +582,13 @@ GetAuthenticatedUserId(void)
 }
 
 /*
- * Return whether the authenticated user was superuser at connection start.
+ * Return whether the authenticated user is currently a superuser.
  */
 bool
 GetAuthenticatedUserIsSuperuser(void)
 {
 	Assert(OidIsValid(AuthenticatedUserId));
-	return AuthenticatedUserIsSuperuser;
+	return superuser_arg(AuthenticatedUserId);
 }
 
 
@@ -741,6 +740,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	HeapTuple	roleTup;
 	Form_pg_authid rform;
 	char	   *rname;
+	bool	   is_superuser;
 
 	/*
 	 * Don't do scans if we're bootstrapping, none of the system catalogs
@@ -780,10 +780,10 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	rname = NameStr(rform->rolname);
 
 	AuthenticatedUserId = roleid;
-	AuthenticatedUserIsSuperuser = rform->rolsuper;
+	is_superuser = rform->rolsuper;
 
 	/* This sets OuterUserId/CurrentUserId too */
-	SetSessionUserId(roleid, AuthenticatedUserIsSuperuser);
+	SetSessionUserId(roleid, is_superuser);
 
 	/* Also mark our PGPROC entry with the authenticated user id */
 	/* (We assume this is an atomic store so no lock is needed) */
@@ -816,7 +816,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 		 * just document that the connection limit is approximate.
 		 */
 		if (rform->rolconnlimit >= 0 &&
-			!AuthenticatedUserIsSuperuser &&
+			!is_superuser &&
 			CountUserBackends(roleid) > rform->rolconnlimit)
 			ereport(FATAL,
 					(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
@@ -828,7 +828,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	SetConfigOption("session_authorization", rname,
 					PGC_BACKEND, PGC_S_OVERRIDE);
 	SetConfigOption("is_superuser",
-					AuthenticatedUserIsSuperuser ? "on" : "off",
+					is_superuser ? "on" : "off",
 					PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
 
 	ReleaseSysCache(roleTup);
@@ -851,7 +851,6 @@ InitializeSessionUserIdStandalone(void)
 	Assert(!OidIsValid(AuthenticatedUserId));
 
 	AuthenticatedUserId = BOOTSTRAP_SUPERUSERID;
-	AuthenticatedUserIsSuperuser = true;
 
 	SetSessionUserId(BOOTSTRAP_SUPERUSERID, true);
 }
-- 
2.34.1



  [text/x-patch] v5-0001-move-session-auth-permission-check.patch (3.9K, ../../CAAvxfHez_S2VpgdXtHe5oYt=cUREFBctdpmd2iVmLHVmJBsTmg@mail.gmail.com/4-v5-0001-move-session-auth-permission-check.patch)
  download | inline diff:
From e24dfa608e69ebf62f6e94626b9c6f4af49b5524 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Sat, 8 Jul 2023 21:35:03 -0700
Subject: [PATCH 1/2] move session auth permission check

---
 src/backend/commands/variable.c   | 23 +++++++++++++++++++++++
 src/backend/utils/init/miscinit.c | 29 ++++++++++-------------------
 src/include/miscadmin.h           |  1 +
 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..f8e308f1d0 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -846,6 +846,29 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 
 	ReleaseSysCache(roleTup);
 
+	/*
+	 * Only a superuser may set auth ID to something other than himself.  Note
+	 * that in case of multiple SETs in a single session, the original
+	 * userid's superuserness is what matters.  But we set the GUC variable
+	 * is_superuser to indicate whether the *current* session userid is a
+	 * superuser.
+	 */
+	if (roleid != GetAuthenticatedUserId() &&
+		!GetAuthenticatedUserIsSuperuser())
+	{
+		if (source == PGC_S_TEST)
+		{
+			ereport(NOTICE,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission will be denied to set session authorization \"%s\"",
+							*newval)));
+			return true;
+		}
+		GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
+		GUC_check_errmsg("permission denied to set session authorization");
+		return false;
+	}
+
 	/* Set up "extra" struct for assign_session_authorization to use */
 	myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra));
 	if (!myextra)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..f5548a0f47 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -582,6 +582,16 @@ GetAuthenticatedUserId(void)
 	return AuthenticatedUserId;
 }
 
+/*
+ * Return whether the authenticated user was superuser at connection start.
+ */
+bool
+GetAuthenticatedUserIsSuperuser(void)
+{
+	Assert(OidIsValid(AuthenticatedUserId));
+	return AuthenticatedUserIsSuperuser;
+}
+
 
 /*
  * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
@@ -888,29 +898,10 @@ system_user(PG_FUNCTION_ARGS)
 
 /*
  * Change session auth ID while running
- *
- * Only a superuser may set auth ID to something other than himself.  Note
- * that in case of multiple SETs in a single session, the original userid's
- * superuserness is what matters.  But we set the GUC variable is_superuser
- * to indicate whether the *current* session userid is a superuser.
- *
- * Note: this is not an especially clean place to do the permission check.
- * It's OK because the check does not require catalog access and can't
- * fail during an end-of-transaction GUC reversion, but we may someday
- * have to push it up into assign_session_authorization.
  */
 void
 SetSessionAuthorization(Oid userid, bool is_superuser)
 {
-	/* Must have authenticated already, else can't make permission check */
-	Assert(OidIsValid(AuthenticatedUserId));
-
-	if (userid != AuthenticatedUserId &&
-		!AuthenticatedUserIsSuperuser)
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("permission denied to set session authorization")));
-
 	SetSessionUserId(userid, is_superuser);
 
 	SetConfigOption("is_superuser",
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 14bd574fc2..11d6e6869d 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -357,6 +357,7 @@ extern Oid	GetUserId(void);
 extern Oid	GetOuterUserId(void);
 extern Oid	GetSessionUserId(void);
 extern Oid	GetAuthenticatedUserId(void);
+extern bool GetAuthenticatedUserIsSuperuser(void);
 extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
 extern void SetUserIdAndSecContext(Oid userid, int sec_context);
 extern bool InLocalUserIdChange(void);
-- 
2.34.1



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Preventing non-superusers from altering session authorization
@ 2023-07-10 00:54  Joseph Koshakow <[email protected]>
  parent: Joseph Koshakow <[email protected]>
  0 siblings, 1 reply; 19+ messages in thread

From: Joseph Koshakow @ 2023-07-10 00:54 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Sun, Jul 9, 2023 at 1:03 PM Joseph Koshakow <[email protected]> wrote:

>> * Only a superuser may set auth ID to something other than himself

> Is "auth ID" the right term here? Maybe something like "Only a
> superuser may set their session authorization/ID to something other
> than their authenticated ID."

>>   But we set the GUC variable
>> * is_superuser to indicate whether the *current* session userid is a
>> * superuser.

> Just a small correction here, I believe the is_superuser GUC is meant
> to indicate whether the current user id is a superuser, not the current
> session user id. We only update is_superuser in SetSessionAuthorization
> because we are also updating the current user id in SetSessionUserId.

I just realized that you moved this comment from
SetSessionAuthorization. I think we should leave the part about setting
the GUC variable is_superuser on top of SetSessionAuthorization since
that's where we actually set the GUC.

Thanks,
Joe Koshakow


^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Preventing non-superusers from altering session authorization
@ 2023-07-10 20:31  Nathan Bossart <[email protected]>
  parent: Joseph Koshakow <[email protected]>
  0 siblings, 1 reply; 19+ messages in thread

From: Nathan Bossart @ 2023-07-10 20:31 UTC (permalink / raw)
  To: Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Sun, Jul 09, 2023 at 08:54:30PM -0400, Joseph Koshakow wrote:
> I just realized that you moved this comment from
> SetSessionAuthorization. I think we should leave the part about setting
> the GUC variable is_superuser on top of SetSessionAuthorization since
> that's where we actually set the GUC.

Okay.  Here's a new patch set in which I believe I've addressed all
feedback.  I didn't keep the GetAuthenticatedUserIsSuperuser() helper
function around, as I didn't see a strong need for it.  And I haven't
touched the "is_superuser" GUC, either.  I figured we can take up any
changes for it in the other thread.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com


Attachments:

  [text/x-diff] v6-0001-Rename-session_auth_is_superuser-to-current_role_.patch (2.7K, ../../20230710203158.GA410521@nathanxps13/2-v6-0001-Rename-session_auth_is_superuser-to-current_role_.patch)
  download | inline diff:
From 691c7a30d86385cca33a7ac43a5d63c4bc39dae1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Sun, 9 Jul 2023 11:28:56 -0700
Subject: [PATCH v6 1/3] Rename session_auth_is_superuser to
 current_role_is_superuser.

Suggested-by: Joseph Koshakow
Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com
---
 src/backend/access/transam/parallel.c | 2 +-
 src/backend/utils/misc/guc_tables.c   | 9 ++++++---
 src/include/utils/guc.h               | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 2b8bc2f58d..12d97998cc 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -326,7 +326,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
 	fps->database_id = MyDatabaseId;
 	fps->authenticated_user_id = GetAuthenticatedUserId();
 	fps->outer_user_id = GetCurrentRoleId();
-	fps->is_superuser = session_auth_is_superuser;
+	fps->is_superuser = current_role_is_superuser;
 	GetUserIdAndSecContext(&fps->current_user_id, &fps->sec_context);
 	GetTempNamespaceState(&fps->temp_namespace_id,
 						  &fps->temp_toast_namespace_id);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c14456060c..93dc2e7680 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -511,7 +511,7 @@ bool		check_function_bodies = true;
  * details.
  */
 bool		default_with_oids = false;
-bool		session_auth_is_superuser;
+bool		current_role_is_superuser;
 
 int			log_min_error_statement = ERROR;
 int			log_min_messages = WARNING;
@@ -1037,13 +1037,16 @@ struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
-		/* Not for general use --- used by SET SESSION AUTHORIZATION */
+		/*
+		 * Not for general use --- used by SET SESSION AUTHORIZATION and SET
+		 * ROLE
+		 */
 		{"is_superuser", PGC_INTERNAL, UNGROUPED,
 			gettext_noop("Shows whether the current user is a superuser."),
 			NULL,
 			GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
 		},
-		&session_auth_is_superuser,
+		&current_role_is_superuser,
 		false,
 		NULL, NULL, NULL
 	},
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d5253c7ed2..223a19f80d 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -250,7 +250,7 @@ extern PGDLLIMPORT bool log_statement_stats;
 extern PGDLLIMPORT bool log_btree_build_stats;
 
 extern PGDLLIMPORT bool check_function_bodies;
-extern PGDLLIMPORT bool session_auth_is_superuser;
+extern PGDLLIMPORT bool current_role_is_superuser;
 
 extern PGDLLIMPORT bool log_duration;
 extern PGDLLIMPORT int log_parameter_max_length;
-- 
2.25.1



  [text/x-diff] v6-0002-Move-session-auth-privilege-check-to-check_sessio.patch (4.1K, ../../20230710203158.GA410521@nathanxps13/3-v6-0002-Move-session-auth-privilege-check-to-check_sessio.patch)
  download | inline diff:
From a08490dd85f0d41830aa06982a5c5ea588ba79d1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Sat, 8 Jul 2023 21:35:03 -0700
Subject: [PATCH v6 2/3] Move session auth privilege check to
 check_session_authorization().

Author: Joseph Koshakow
Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com
---
 src/backend/commands/variable.c   | 21 +++++++++++++++++++++
 src/backend/utils/init/miscinit.c | 30 ++++++++++++------------------
 src/include/miscadmin.h           |  1 +
 3 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..b8a75012a5 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -846,6 +846,27 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 
 	ReleaseSysCache(roleTup);
 
+	/*
+	 * Only superusers may SET SESSION AUTHORIZATION to roles other than
+	 * itself.  Note that in case of multiple SETs in a single session, the
+	 * original authenticated user's superuserness is what matters.
+	 */
+	if (roleid != GetAuthenticatedUserId() &&
+		!GetAuthenticatedUserIsSuperuser())
+	{
+		if (source == PGC_S_TEST)
+		{
+			ereport(NOTICE,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission will be denied to set session authorization \"%s\"",
+							*newval)));
+			return true;
+		}
+		GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
+		GUC_check_errmsg("permission denied to set session authorization");
+		return false;
+	}
+
 	/* Set up "extra" struct for assign_session_authorization to use */
 	myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra));
 	if (!myextra)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..64545bc373 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -582,6 +582,16 @@ GetAuthenticatedUserId(void)
 	return AuthenticatedUserId;
 }
 
+/*
+ * Return whether the authenticated user was superuser at connection start.
+ */
+bool
+GetAuthenticatedUserIsSuperuser(void)
+{
+	Assert(OidIsValid(AuthenticatedUserId));
+	return AuthenticatedUserIsSuperuser;
+}
+
 
 /*
  * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
@@ -889,28 +899,12 @@ system_user(PG_FUNCTION_ARGS)
 /*
  * Change session auth ID while running
  *
- * Only a superuser may set auth ID to something other than himself.  Note
- * that in case of multiple SETs in a single session, the original userid's
- * superuserness is what matters.  But we set the GUC variable is_superuser
- * to indicate whether the *current* session userid is a superuser.
- *
- * Note: this is not an especially clean place to do the permission check.
- * It's OK because the check does not require catalog access and can't
- * fail during an end-of-transaction GUC reversion, but we may someday
- * have to push it up into assign_session_authorization.
+ * Note that we set the GUC variable is_superuser to indicate whether the
+ * current role is a superuser.
  */
 void
 SetSessionAuthorization(Oid userid, bool is_superuser)
 {
-	/* Must have authenticated already, else can't make permission check */
-	Assert(OidIsValid(AuthenticatedUserId));
-
-	if (userid != AuthenticatedUserId &&
-		!AuthenticatedUserIsSuperuser)
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("permission denied to set session authorization")));
-
 	SetSessionUserId(userid, is_superuser);
 
 	SetConfigOption("is_superuser",
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 14bd574fc2..11d6e6869d 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -357,6 +357,7 @@ extern Oid	GetUserId(void);
 extern Oid	GetOuterUserId(void);
 extern Oid	GetSessionUserId(void);
 extern Oid	GetAuthenticatedUserId(void);
+extern bool GetAuthenticatedUserIsSuperuser(void);
 extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
 extern void SetUserIdAndSecContext(Oid userid, int sec_context);
 extern bool InLocalUserIdChange(void);
-- 
2.25.1



  [text/x-diff] v6-0003-Prevent-non-superusers-from-altering-session-auth.patch (6.3K, ../../20230710203158.GA410521@nathanxps13/4-v6-0003-Prevent-non-superusers-from-altering-session-auth.patch)
  download | inline diff:
From 7d54a90606a35571151db433df94976e2024c205 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Sun, 9 Jul 2023 15:01:53 -0700
Subject: [PATCH v6 3/3] Prevent non-superusers from altering session auth

Previously, if a user connected with as a role that had the superuser
attribute, then they could always execute a SET SESSION AUTHORIZATION
statement for the duration of their session. Even if the role was
altered to set superuser to false, the user was still allowed to
execute SET SESSION AUTHORIZATION. This allowed them to set their
session role to some other superuser and effectively regain the
superuser privileges. They could even reset their own superuser
attribute to true.

This commit alters the privilege checks for SET SESSION AUTHORIZATION
such that a user can only execute it if the role they connected with is
currently a superuser. This prevents users from regaining superuser
privileges after it has been revoked.

Author: Joseph Koshakow
Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com
---
 doc/src/sgml/ref/set_session_auth.sgml |  2 +-
 src/backend/commands/variable.c        |  2 +-
 src/backend/utils/init/miscinit.c      | 28 ++++++++------------------
 src/include/miscadmin.h                |  1 -
 4 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml
index f8fcafc194..94adab2468 100644
--- a/doc/src/sgml/ref/set_session_auth.sgml
+++ b/doc/src/sgml/ref/set_session_auth.sgml
@@ -51,7 +51,7 @@ RESET SESSION AUTHORIZATION
 
   <para>
    The session user identifier can be changed only if the initial session
-   user (the <firstterm>authenticated user</firstterm>) had the
+   user (the <firstterm>authenticated user</firstterm>) has the
    superuser privilege.  Otherwise, the command is accepted only if it
    specifies the authenticated user name.
   </para>
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index b8a75012a5..8404818d09 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -852,7 +852,7 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 	 * original authenticated user's superuserness is what matters.
 	 */
 	if (roleid != GetAuthenticatedUserId() &&
-		!GetAuthenticatedUserIsSuperuser())
+		!superuser_arg(GetAuthenticatedUserId()))
 	{
 		if (source == PGC_S_TEST)
 		{
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 64545bc373..1e671c560c 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -467,8 +467,8 @@ ChangeToDataDir(void)
  * AuthenticatedUserId is determined at connection start and never changes.
  *
  * SessionUserId is initially the same as AuthenticatedUserId, but can be
- * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserIsSuperuser).
- * This is the ID reported by the SESSION_USER SQL function.
+ * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserId is a
+ * superuser).  This is the ID reported by the SESSION_USER SQL function.
  *
  * OuterUserId is the current user ID in effect at the "outer level" (outside
  * any transaction or function).  This is initially the same as SessionUserId,
@@ -492,8 +492,7 @@ static Oid	OuterUserId = InvalidOid;
 static Oid	CurrentUserId = InvalidOid;
 static const char *SystemUser = NULL;
 
-/* We also have to remember the superuser state of some of these levels */
-static bool AuthenticatedUserIsSuperuser = false;
+/* We also have to remember the superuser state of the session user */
 static bool SessionUserIsSuperuser = false;
 
 static int	SecurityRestrictionContext = 0;
@@ -582,16 +581,6 @@ GetAuthenticatedUserId(void)
 	return AuthenticatedUserId;
 }
 
-/*
- * Return whether the authenticated user was superuser at connection start.
- */
-bool
-GetAuthenticatedUserIsSuperuser(void)
-{
-	Assert(OidIsValid(AuthenticatedUserId));
-	return AuthenticatedUserIsSuperuser;
-}
-
 
 /*
  * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
@@ -741,6 +730,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	HeapTuple	roleTup;
 	Form_pg_authid rform;
 	char	   *rname;
+	bool		is_superuser;
 
 	/*
 	 * Don't do scans if we're bootstrapping, none of the system catalogs
@@ -780,10 +770,10 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	rname = NameStr(rform->rolname);
 
 	AuthenticatedUserId = roleid;
-	AuthenticatedUserIsSuperuser = rform->rolsuper;
+	is_superuser = rform->rolsuper;
 
 	/* This sets OuterUserId/CurrentUserId too */
-	SetSessionUserId(roleid, AuthenticatedUserIsSuperuser);
+	SetSessionUserId(roleid, is_superuser);
 
 	/* Also mark our PGPROC entry with the authenticated user id */
 	/* (We assume this is an atomic store so no lock is needed) */
@@ -816,7 +806,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 		 * just document that the connection limit is approximate.
 		 */
 		if (rform->rolconnlimit >= 0 &&
-			!AuthenticatedUserIsSuperuser &&
+			!is_superuser &&
 			CountUserBackends(roleid) > rform->rolconnlimit)
 			ereport(FATAL,
 					(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
@@ -828,7 +818,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	SetConfigOption("session_authorization", rname,
 					PGC_BACKEND, PGC_S_OVERRIDE);
 	SetConfigOption("is_superuser",
-					AuthenticatedUserIsSuperuser ? "on" : "off",
+					is_superuser ? "on" : "off",
 					PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
 
 	ReleaseSysCache(roleTup);
@@ -851,8 +841,6 @@ InitializeSessionUserIdStandalone(void)
 	Assert(!OidIsValid(AuthenticatedUserId));
 
 	AuthenticatedUserId = BOOTSTRAP_SUPERUSERID;
-	AuthenticatedUserIsSuperuser = true;
-
 	SetSessionUserId(BOOTSTRAP_SUPERUSERID, true);
 }
 
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 11d6e6869d..14bd574fc2 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -357,7 +357,6 @@ extern Oid	GetUserId(void);
 extern Oid	GetOuterUserId(void);
 extern Oid	GetSessionUserId(void);
 extern Oid	GetAuthenticatedUserId(void);
-extern bool GetAuthenticatedUserIsSuperuser(void);
 extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
 extern void SetUserIdAndSecContext(Oid userid, int sec_context);
 extern bool InLocalUserIdChange(void);
-- 
2.25.1



^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Preventing non-superusers from altering session authorization
@ 2023-07-10 20:46  Joseph Koshakow <[email protected]>
  parent: Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 19+ messages in thread

From: Joseph Koshakow @ 2023-07-10 20:46 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Mon, Jul 10, 2023 at 4:32 PM Nathan Bossart <[email protected]>
wrote:
> Okay.  Here's a new patch set in which I believe I've addressed all
> feedback.  I didn't keep the GetAuthenticatedUserIsSuperuser() helper
> function around, as I didn't see a strong need for it.

Thanks, I think the patch set looks good to go!

> And I haven't
> touched the "is_superuser" GUC, either.  I figured we can take up any
> changes for it in the other thread.

Yeah, I think that makes sense.

Thanks,
Joe Koshakow


^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Preventing non-superusers from altering session authorization
@ 2023-07-10 20:49  Nathan Bossart <[email protected]>
  parent: Joseph Koshakow <[email protected]>
  0 siblings, 1 reply; 19+ messages in thread

From: Nathan Bossart @ 2023-07-10 20:49 UTC (permalink / raw)
  To: Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Mon, Jul 10, 2023 at 04:46:07PM -0400, Joseph Koshakow wrote:
> Thanks, I think the patch set looks good to go!

Great.  I'm going to wait a few more days in case anyone has additional
feedback, but otherwise I intend to commit this shortly.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Preventing non-superusers from altering session authorization
@ 2023-07-13 04:37  Nathan Bossart <[email protected]>
  parent: Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 19+ messages in thread

From: Nathan Bossart @ 2023-07-13 04:37 UTC (permalink / raw)
  To: Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Mon, Jul 10, 2023 at 01:49:55PM -0700, Nathan Bossart wrote:
> Great.  I'm going to wait a few more days in case anyone has additional
> feedback, but otherwise I intend to commit this shortly.

I've committed 0001 for now.  I'm hoping to commit the other two patches
within the next couple of days.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com






^ permalink  raw  reply  [nested|flat] 19+ messages in thread

* Re: Preventing non-superusers from altering session authorization
@ 2023-07-14 04:16  Nathan Bossart <[email protected]>
  parent: Nathan Bossart <[email protected]>
  0 siblings, 0 replies; 19+ messages in thread

From: Nathan Bossart @ 2023-07-14 04:16 UTC (permalink / raw)
  To: Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

On Wed, Jul 12, 2023 at 09:37:57PM -0700, Nathan Bossart wrote:
> On Mon, Jul 10, 2023 at 01:49:55PM -0700, Nathan Bossart wrote:
>> Great.  I'm going to wait a few more days in case anyone has additional
>> feedback, but otherwise I intend to commit this shortly.
> 
> I've committed 0001 for now.  I'm hoping to commit the other two patches
> within the next couple of days.

Committed.  I dwelled on whether to proceed with this change because it
doesn't completely solve the originally-stated problem; i.e., a role that
has changed its session authorization before losing superuser can still
take advantage of the privileges of the target role, which might include
reaquiring superuser.  However, I think SET ROLE is subject to basically
the same problem, and I'd argue that this change is strictly an
improvement, if for no other reason than it makes SET SESSION AUTHORIZATION
more consistent with SET ROLE.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

^ permalink  raw  reply  [nested|flat] 19+ messages in thread

end of thread, other threads:[~2023-07-14 04:16 UTC | newest]

Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-09-07 10:13 [PATCH 2/2] Add monitoring aid for max_replication_slots. Kyotaro Horiguchi <[email protected]>
2022-01-17 06:54 [PATCH 6/8] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 16/19] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 17/23] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 20/25] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 4/7] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 16/19] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 17/23] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 16/19] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 17/21] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 7/9] cirrus: code coverage Justin Pryzby <[email protected]>
2022-01-17 06:54 [PATCH 17/21] cirrus: code coverage Justin Pryzby <[email protected]>
2023-07-09 17:03 Re: Preventing non-superusers from altering session authorization Joseph Koshakow <[email protected]>
2023-07-10 00:54 ` Re: Preventing non-superusers from altering session authorization Joseph Koshakow <[email protected]>
2023-07-10 20:31   ` Re: Preventing non-superusers from altering session authorization Nathan Bossart <[email protected]>
2023-07-10 20:46     ` Re: Preventing non-superusers from altering session authorization Joseph Koshakow <[email protected]>
2023-07-10 20:49       ` Re: Preventing non-superusers from altering session authorization Nathan Bossart <[email protected]>
2023-07-13 04:37         ` Re: Preventing non-superusers from altering session authorization Nathan Bossart <[email protected]>
2023-07-14 04:16           ` Re: Preventing non-superusers from altering session authorization Nathan Bossart <[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