public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 1/1] Use has_privs_of_role for samerole, samegroup, and + in pg_hba.conf.
8+ messages / 5 participants
[nested] [flat]
* [PATCH v2 1/1] Use has_privs_of_role for samerole, samegroup, and + in pg_hba.conf.
@ 2022-04-01 18:40 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Nathan Bossart @ 2022-04-01 18:40 UTC (permalink / raw)
6198420 ensured that has_privs_of_role() is used for predefined
roles, which means that the role privilege inheritance hierarchy is
checked instead of mere role membership. However, inheritance is
still not respected for pg_hba.conf. This change alters the
authentication logic to consider role privileges instead of just
role membership.
Do not back-patch.
Author: Nathan Bossart
---
doc/src/sgml/client-auth.sgml | 18 ++---
src/backend/libpq/hba.c | 19 ++---
src/backend/utils/adt/acl.c | 23 ++++++
src/include/utils/acl.h | 1 +
src/test/authentication/meson.build | 1 +
src/test/authentication/t/004_privs.pl | 101 +++++++++++++++++++++++++
6 files changed, 145 insertions(+), 18 deletions(-)
create mode 100644 src/test/authentication/t/004_privs.pl
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c6f1b70fd3..b06b57f169 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -221,12 +221,12 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl
The value <literal>sameuser</literal> specifies that the record
matches if the requested database has the same name as the
requested user. The value <literal>samerole</literal> specifies that
- the requested user must be a member of the role with the same
+ the requested user must have privileges of the role with the same
name as the requested database. (<literal>samegroup</literal> is an
obsolete but still accepted spelling of <literal>samerole</literal>.)
- Superusers are not considered to be members of a role for the
- purposes of <literal>samerole</literal> unless they are explicitly
- members of the role, directly or indirectly, and not just by
+ Superusers are not considered to have privileges of a role for the
+ purposes of <literal>samerole</literal> unless they explicitly have
+ privileges of the role, directly or indirectly, and not just by
virtue of being a superuser.
The value <literal>replication</literal> specifies that the record
matches if a physical replication connection is requested, however, it
@@ -252,10 +252,10 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl
database user, or a group name preceded by <literal>+</literal>.
(Recall that there is no real distinction between users and groups
in <productname>PostgreSQL</productname>; a <literal>+</literal> mark really means
- <quote>match any of the roles that are directly or indirectly members
+ <quote>match any of the roles that directly or indirectly have privileges
of this role</quote>, while a name without a <literal>+</literal> mark matches
only that specific role.) For this purpose, a superuser is only
- considered to be a member of a role if they are explicitly a member
+ considered to have privileges of a role if they explicitly have privileges
of the role, directly or indirectly, and not just by virtue of
being a superuser.
Multiple user names can be supplied by separating them with commas.
@@ -788,9 +788,9 @@ host all all 192.168.0.0/16 ident map=omicro
# If these are the only three lines for local connections, they will
# allow local users to connect only to their own databases (databases
# with the same name as their database user name) except for administrators
-# and members of role "support", who can connect to all databases. The file
-# $PGDATA/admins contains a list of names of administrators. Passwords
-# are required in all cases.
+# and roles with privileges of role "support", who can connect to all
+# databases. The file $PGDATA/admins contains a list of names of
+# administrators. Passwords are required in all cases.
#
# TYPE DATABASE USER ADDRESS METHOD
local sameuser all md5
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 4637426d62..faa675f4af 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -547,13 +547,13 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
/*
- * Does user belong to role?
+ * Does user have privileges of role?
*
* userid is the OID of the role given as the attempted login identifier.
- * We check to see if it is a member of the specified role name.
+ * We check to see if it has privileges of the specified role name.
*/
static bool
-is_member(Oid userid, const char *role)
+has_privs(Oid userid, const char *role)
{
Oid roleid;
@@ -566,11 +566,12 @@ is_member(Oid userid, const char *role)
return false; /* if target role not exist, say "no" */
/*
- * See if user is directly or indirectly a member of role. For this
- * purpose, a superuser is not considered to be automatically a member of
- * the role, so group auth only applies to explicit membership.
+ * See if user directly or indirectly has privileges of role. For this
+ * purpose, a superuser is not considered to automatically have
+ * privileges of the role, so group auth only applies to explicit
+ * privileges.
*/
- return is_member_of_role_nosuper(userid, roleid);
+ return has_privs_of_role_nosuper(userid, roleid);
}
/*
@@ -587,7 +588,7 @@ check_role(const char *role, Oid roleid, List *tokens)
tok = lfirst(cell);
if (!tok->quoted && tok->string[0] == '+')
{
- if (is_member(roleid, tok->string + 1))
+ if (has_privs(roleid, tok->string + 1))
return true;
}
else if (token_matches(tok, role) ||
@@ -628,7 +629,7 @@ check_db(const char *dbname, const char *role, Oid roleid, List *tokens)
else if (token_is_keyword(tok, "samegroup") ||
token_is_keyword(tok, "samerole"))
{
- if (is_member(roleid, dbname))
+ if (has_privs(roleid, dbname))
return true;
}
else if (token_is_keyword(tok, "replication"))
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 4fac402e5b..cb5587d926 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -4928,6 +4928,29 @@ has_privs_of_role(Oid member, Oid role)
}
+/*
+ * Does member have the privileges of role, not considering superuserness?
+ *
+ * This is identical to has_privs_of_role except we ignore superuser
+ * status.
+ */
+bool
+has_privs_of_role_nosuper(Oid member, Oid role)
+{
+ /* Fast path for simple case */
+ if (member == role)
+ return true;
+
+ /*
+ * Find all the roles that member has the privileges of, including
+ * multi-level recursion, then see if target role is any one of them.
+ */
+ return list_member_oid(roles_is_member_of(member, ROLERECURSE_PRIVS,
+ InvalidOid, NULL),
+ role);
+}
+
+
/*
* Is member a member of role (directly or indirectly)?
*
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 9a4df3a5da..eded5e0f96 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -209,6 +209,7 @@ extern AclMode aclmask(const Acl *acl, Oid roleid, Oid ownerId,
extern int aclmembers(const Acl *acl, Oid **roleids);
extern bool has_privs_of_role(Oid member, Oid role);
+extern bool has_privs_of_role_nosuper(Oid member, Oid role);
extern bool is_member_of_role(Oid member, Oid role);
extern bool is_member_of_role_nosuper(Oid member, Oid role);
extern bool is_admin_of_role(Oid member, Oid role);
diff --git a/src/test/authentication/meson.build b/src/test/authentication/meson.build
index c2b48c43c9..a98b19158c 100644
--- a/src/test/authentication/meson.build
+++ b/src/test/authentication/meson.build
@@ -7,6 +7,7 @@ tests += {
't/001_password.pl',
't/002_saslprep.pl',
't/003_peer.pl',
+ 't/004_privs.pl',
],
},
}
diff --git a/src/test/authentication/t/004_privs.pl b/src/test/authentication/t/004_privs.pl
new file mode 100644
index 0000000000..40b7e9f6e5
--- /dev/null
+++ b/src/test/authentication/t/004_privs.pl
@@ -0,0 +1,101 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+# Tests for role inheritance with pg_hba.conf.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Delete pg_hba.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_hba
+{
+ my $node = shift;
+ my $database = shift;
+ my $role = shift;
+ my $hba_method = shift;
+
+ unlink($node->data_dir . '/pg_hba.conf');
+ $node->append_conf('pg_hba.conf', "local $database $role $hba_method");
+ $node->reload;
+ return;
+}
+
+# Test access for a connection string, useful to wrap all tests into one.
+# Extra named parameters are passed to connect_ok/fails as-is.
+sub test_conn
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+ my ($node, $connstr, $method, $expected_res, %params) = @_;
+ my $status_string = 'failed';
+ $status_string = 'success' if ($expected_res eq 0);
+
+ my $testname =
+ "authentication $status_string for method $method, connstr $connstr";
+
+ if ($expected_res eq 0)
+ {
+ $node->connect_ok($connstr, $testname, %params);
+ }
+ else
+ {
+ # No checks of the error message, only the status code.
+ $node->connect_fails($connstr, $testname, %params);
+ }
+}
+
+# Initialize primary node
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init;
+$node->append_conf('postgresql.conf', "log_connections = on\n");
+$node->start;
+
+# Create database and roles for tests
+$node->safe_psql('postgres', "CREATE DATABASE role1;");
+$node->safe_psql('postgres', "CREATE ROLE role1 LOGIN PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE role2 LOGIN SUPERUSER INHERIT IN ROLE role1 PASSWORD 'pass';");
+$node->safe_psql('postgres', "CREATE ROLE role3 LOGIN SUPERUSER NOINHERIT IN ROLE role1 PASSWORD 'pass';");
+
+# Test role inheritance is respected for +
+$ENV{"PGPASSWORD"} = 'pass';
+reset_pg_hba($node, 'all', '+role1', 'scram-sha-256');
+test_conn($node, 'user=role1', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role1" method=scram-sha-256/]);
+test_conn($node, 'user=role2', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role2" method=scram-sha-256/]);
+test_conn($node, 'user=role3', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="role3" method=scram-sha-256/]);
+
+# Test role inheritance is respected for samerole
+$ENV{"PGDATABASE"} = 'role1';
+reset_pg_hba($node, 'samerole', 'all', 'scram-sha-256');
+test_conn($node, 'user=role1', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role1" method=scram-sha-256/]);
+test_conn($node, 'user=role2', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role2" method=scram-sha-256/]);
+test_conn($node, 'user=role3', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="role3" method=scram-sha-256/]);
+
+# Test role inheritance is respected for samegroup
+reset_pg_hba($node, 'samegroup', 'all', 'scram-sha-256');
+test_conn($node, 'user=role1', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role1" method=scram-sha-256/]);
+test_conn($node, 'user=role2', 'scram-sha-256', 0,
+ log_like =>
+ [qr/connection authenticated: identity="role2" method=scram-sha-256/]);
+test_conn($node, 'user=role3', 'scram-sha-256', 2,
+ log_unlike =>
+ [qr/connection authenticated: identity="role3" method=scram-sha-256/]);
+
+done_testing();
--
2.25.1
--ZGiS0Q5IWpPtfppv--
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
@ 2025-04-13 15:33 Noah Misch <[email protected]>
2025-04-13 15:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Noah Misch @ 2025-04-13 15:33 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Sat, Apr 05, 2025 at 07:09:58PM -0700, Noah Misch wrote:
> On Sun, Apr 06, 2025 at 07:42:02AM +0900, Michael Paquier wrote:
> > On Sat, Apr 05, 2025 at 12:13:39PM -0700, Noah Misch wrote:
> > > Since the 2025-02 releases made non-toy-size archive recoveries fail easily,
> > > that's not enough. If the proposed 3-second test is the wrong thing, what
> > > instead?
> >
> > I don't have a good idea about that in ~16, TBH, but I am sure to not
> > be a fan of the low reproducibility rate of this test as proposed.
> > It's not perfect, but as the design to fix the original race condition
> > has been introduced in v15, why not begin with a test in 17~ using
> > some injection points?
>
> Two reasons:
>
> a) The fix ended calls to the whole range of relevant code. Hence, the
> injection point placement that would have been relevant before the fix
> isn't reached. In other words, there's no right place for the injection
> point. (The place for the injection point would be in durable_rename(), in
> the checkpointer. After the fix, the checkpointer just doesn't call
> durable_rename().)
>
> b) Stochastic tests catch defects beyond the specific one the test author
> targeted. An injection point test is less likely to do that. (That said,
> with reason (a), there's no known injection point test design to compete
> with the stochastic design.)
Tom and Michael, do you still object to the test addition, or not? If there
are no new or renewed objections by 2025-04-20, I'll proceed to add the test.
As another data point, raising the runtime from 3s to 17s makes it reproduce
the problem 25% of the time. You can imagine a plot with axes of runtime and
percent detection. One can pick any point on that plot's curve. Given how
little wall time it takes for the buildfarm and CI to reach a few hundred
runs, I like the trade-off of 3s runtime and 1% detection. In particular, I
like it better than 17s runtime for 25% detection. How do you see it?
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2025-04-13 15:33 Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
@ 2025-04-13 15:51 ` Tom Lane <[email protected]>
2025-04-14 00:19 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2025-04-13 15:51 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Michael Paquier <[email protected]>; Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
Noah Misch <[email protected]> writes:
> Tom and Michael, do you still object to the test addition, or not? If there
> are no new or renewed objections by 2025-04-20, I'll proceed to add the test.
While I still don't love it, I don't have a better proposal.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2025-04-13 15:33 Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-13 15:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
@ 2025-04-14 00:19 ` Michael Paquier <[email protected]>
2025-04-20 21:53 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2025-04-14 00:19 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Sun, Apr 13, 2025 at 11:51:57AM -0400, Tom Lane wrote:
> Noah Misch <[email protected]> writes:
> > Tom and Michael, do you still object to the test addition, or not? If there
> > are no new or renewed objections by 2025-04-20, I'll proceed to add the test.
>
> While I still don't love it, I don't have a better proposal.
No objections from here to use your proposal for the time being on all
the branches. I am planning to spend some cycles thinking about a
better alternative, but I doubt that this will be portable down to
v13.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2025-04-13 15:33 Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-13 15:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2025-04-14 00:19 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2025-04-20 21:53 ` Noah Misch <[email protected]>
2025-04-20 22:15 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-25 19:35 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Andres Freund <[email protected]>
0 siblings, 2 replies; 8+ messages in thread
From: Noah Misch @ 2025-04-20 21:53 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Mon, Apr 14, 2025 at 09:19:35AM +0900, Michael Paquier wrote:
> On Sun, Apr 13, 2025 at 11:51:57AM -0400, Tom Lane wrote:
> > Noah Misch <[email protected]> writes:
> > > Tom and Michael, do you still object to the test addition, or not? If there
> > > are no new or renewed objections by 2025-04-20, I'll proceed to add the test.
Pushed as commit 714bd9e. The failure so far is
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-04-20%2015%3A36%3A35
with these highlights:
pg_ctl: server does not shut down
2025-04-20 17:27:35.735 UTC [1576688][postmaster][:0] LOG: received immediate shutdown request
2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] FATAL: archive command was terminated by signal 3: Quit
2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] DETAIL: The failed archive command was: cp "pg_wal/00000001000000000000006D" "/home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/recovery/045_archive_restartpoint/data/t_045_archive_restartpoint_primary_data/archives/00000001000000000000006D"
The checkpoints and WAL creation took 30s, but archiving was only 20% done
(based on file name 00000001000000000000006D) at the 360s PGCTLTIMEOUT. I can
reproduce this if I test with valgrind --trace-children=yes. With my normal
valgrind settings, the whole test file takes only 18s. I recommend one of
these changes to skink:
- Add --trace-children-skip='/bin/*,/usr/bin/*' so valgrind doesn't instrument
"sh" and "cp" commands.
- Remove --trace-children=yes
Andres, what do you think about making one of those skink configuration
changes? Alternatively, I could make the test poll until archiving catches
up. However, that would take skink about 30min, and I expect little value
from 30min of valgrind instrumenting the "cp" command.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2025-04-13 15:33 Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-13 15:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2025-04-14 00:19 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2025-04-20 21:53 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
@ 2025-04-20 22:15 ` Noah Misch <[email protected]>
1 sibling, 0 replies; 8+ messages in thread
From: Noah Misch @ 2025-04-20 22:15 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Sun, Apr 20, 2025 at 02:53:39PM -0700, Noah Misch wrote:
> On Mon, Apr 14, 2025 at 09:19:35AM +0900, Michael Paquier wrote:
> > On Sun, Apr 13, 2025 at 11:51:57AM -0400, Tom Lane wrote:
> > > Noah Misch <[email protected]> writes:
> > > > Tom and Michael, do you still object to the test addition, or not? If there
> > > > are no new or renewed objections by 2025-04-20, I'll proceed to add the test.
>
> Pushed as commit 714bd9e. The failure so far is
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-04-20%2015%3A36%3A35
> with these highlights:
>
> pg_ctl: server does not shut down
>
> 2025-04-20 17:27:35.735 UTC [1576688][postmaster][:0] LOG: received immediate shutdown request
> 2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] FATAL: archive command was terminated by signal 3: Quit
> 2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] DETAIL: The failed archive command was: cp "pg_wal/00000001000000000000006D" "/home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/recovery/045_archive_restartpoint/data/t_045_archive_restartpoint_primary_data/archives/00000001000000000000006D"
>
> The checkpoints and WAL creation took 30s, but archiving was only 20% done
> (based on file name 00000001000000000000006D) at the 360s PGCTLTIMEOUT. I can
> reproduce this if I test with valgrind --trace-children=yes. With my normal
> valgrind settings, the whole test file takes only 18s. I recommend one of
> these changes to skink:
>
> - Add --trace-children-skip='/bin/*,/usr/bin/*' so valgrind doesn't instrument
> "sh" and "cp" commands.
> - Remove --trace-children=yes
I gave that more thought. One can be more surgical than that, via
--trace-children-skip-by-arg='*cp "*' or similar. My previous message's two
options stop valgrind instrumentation at boundaries like pg_dumpall calling
system(pg_dump ...), since that execs /bin/sh to run pg_dump. If we wanted to
make it even more explicit and surgical, skink could use
--trace-children-skip-by-arg='*valgrind-ignore-child*' combined with:
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1404 +1404 @@ sub enable_restoring
- : qq{cp "$path/%f" "%p"};
+ : qq{cp "$path/%f" "%p" # valgrind-ignore-child};
@@ -1474 +1474 @@ sub enable_archiving
- : qq{cp "%p" "$path/%f"};
+ : qq{cp "%p" "$path/%f" # valgrind-ignore-child};
What's your preference?
> Andres, what do you think about making one of those skink configuration
> changes? Alternatively, I could make the test poll until archiving catches
> up. However, that would take skink about 30min, and I expect little value
> from 30min of valgrind instrumenting the "cp" command.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2025-04-13 15:33 Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-13 15:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2025-04-14 00:19 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2025-04-20 21:53 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
@ 2025-04-25 19:35 ` Andres Freund <[email protected]>
2025-04-26 23:03 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: Andres Freund @ 2025-04-25 19:35 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
Hi,
On 2025-04-20 14:53:39 -0700, Noah Misch wrote:
> On Mon, Apr 14, 2025 at 09:19:35AM +0900, Michael Paquier wrote:
> > On Sun, Apr 13, 2025 at 11:51:57AM -0400, Tom Lane wrote:
> > > Noah Misch <[email protected]> writes:
> > > > Tom and Michael, do you still object to the test addition, or not? If there
> > > > are no new or renewed objections by 2025-04-20, I'll proceed to add the test.
>
> Pushed as commit 714bd9e. The failure so far is
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-04-20%2015%3A36%3A35
> with these highlights:
>
> pg_ctl: server does not shut down
>
> 2025-04-20 17:27:35.735 UTC [1576688][postmaster][:0] LOG: received immediate shutdown request
> 2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] FATAL: archive command was terminated by signal 3: Quit
> 2025-04-20 17:27:35.969 UTC [1577386][archiver][:0] DETAIL: The failed archive command was: cp "pg_wal/00000001000000000000006D" "/home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/recovery/045_archive_restartpoint/data/t_045_archive_restartpoint_primary_data/archives/00000001000000000000006D"
>
> The checkpoints and WAL creation took 30s, but archiving was only 20% done
> (based on file name 00000001000000000000006D) at the 360s PGCTLTIMEOUT.
Huh. That seems surprisingly slow, even for valgrind. I guess it's one more
example for why the single-threaded archiving approach sucks so badly :)
> I can reproduce this if I test with valgrind --trace-children=yes. With my
> normal valgrind settings, the whole test file takes only 18s. I recommend
> one of these changes to skink:
>
> - Add --trace-children-skip='/bin/*,/usr/bin/*' so valgrind doesn't instrument
> "sh" and "cp" commands.
> - Remove --trace-children=yes
Hm. I think I used --trace-children=yes because I was thinking it was required
to track forks. But a newer version of valgrind's man page has an important
clarification:
--trace-children=<yes|no> [default: no]
When enabled, Valgrind will trace into sub-processes initiated via the exec system call. This is necessary for multi-process programs.
Note that Valgrind does trace into the child of a fork (it would be difficult not to, since fork makes an identical copy of a process), so this
option is arguably badly named. However, most children of fork calls immediately call exec anyway.
So there doesn't seem to be much point in using --trace-children=yes.
> Andres, what do you think about making one of those skink configuration
> changes? Alternatively, I could make the test poll until archiving catches
> up. However, that would take skink about 30min, and I expect little value
> from 30min of valgrind instrumenting the "cp" command.
I just changed the config to --trace-children=no. There already is a valgrind
run in progress, so it won't be in effect for the next run.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2025-04-13 15:33 Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-13 15:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2025-04-14 00:19 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2025-04-20 21:53 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-25 19:35 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Andres Freund <[email protected]>
@ 2025-04-26 23:03 ` Noah Misch <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Noah Misch @ 2025-04-26 23:03 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Fri, Apr 25, 2025 at 03:35:06PM -0400, Andres Freund wrote:
> On 2025-04-20 14:53:39 -0700, Noah Misch wrote:
> > The checkpoints and WAL creation took 30s, but archiving was only 20% done
> > (based on file name 00000001000000000000006D) at the 360s PGCTLTIMEOUT.
>
> Huh. That seems surprisingly slow, even for valgrind. I guess it's one more
> example for why the single-threaded archiving approach sucks so badly :)
Yes! I also didn't expect that v14/v13 would run much faster.
> I just changed the config to --trace-children=no. There already is a valgrind
> run in progress, so it won't be in effect for the next run.
Works for me. I see that resolved things.
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-04-26 23:03 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-01 18:40 [PATCH v2 1/1] Use has_privs_of_role for samerole, samegroup, and + in pg_hba.conf. Nathan Bossart <[email protected]>
2025-04-13 15:33 Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-13 15:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Tom Lane <[email protected]>
2025-04-14 00:19 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2025-04-20 21:53 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-20 22:15 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[email protected]>
2025-04-25 19:35 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Andres Freund <[email protected]>
2025-04-26 23:03 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Noah Misch <[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