public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v5 2/2] Improve more insufficient-privileges error messages.
10+ messages / 3 participants
[nested] [flat]
* [PATCH v5 2/2] Improve more insufficient-privileges error messages.
@ 2023-01-27 22:52 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2023-01-27 22:52 UTC (permalink / raw)
---
contrib/file_fdw/expected/file_fdw.out | 3 ++-
contrib/file_fdw/file_fdw.c | 8 ++++++--
contrib/test_decoding/expected/permissions.out | 12 ++++++++----
src/backend/backup/basebackup_server.c | 4 +++-
src/backend/catalog/objectaddress.c | 16 +++++++++++-----
src/backend/commands/copy.c | 12 +++++++++---
src/backend/replication/slot.c | 5 +++--
src/backend/storage/ipc/procarray.c | 4 +++-
src/backend/storage/ipc/signalfuncs.c | 16 ++++++++++++----
src/backend/tcop/utility.c | 5 ++++-
src/backend/utils/init/miscinit.c | 4 ++++
src/backend/utils/init/postinit.c | 8 +++++---
src/backend/utils/misc/guc.c | 15 +++++++++------
.../dummy_seclabel/expected/dummy_seclabel.out | 3 ++-
.../modules/unsafe_tests/expected/rolenames.out | 3 ++-
src/test/regress/expected/create_role.out | 3 ++-
16 files changed, 85 insertions(+), 36 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 36d76ba26c..f67a33dace 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -474,7 +474,8 @@ ALTER FOREIGN TABLE agg_text OWNER TO regress_file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
SET ROLE regress_file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
-ERROR: only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table
+ERROR: permission denied to set the "filename" option of a file_fdw foreign table
+DETAIL: You must have privileges of the "pg_read_server_files" role to set this option.
SET ROLE regress_file_fdw_superuser;
-- cleanup
RESET ROLE;
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 8ccc167548..b00fd0a5c7 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -278,13 +278,17 @@ file_fdw_validator(PG_FUNCTION_ARGS)
!has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table")));
+ errmsg("permission denied to set the \"filename\" option of a file_fdw foreign table"),
+ errdetail("You must have privileges of the \"%s\" role to set this option.",
+ "pg_read_server_files")));
if (strcmp(def->defname, "program") == 0 &&
!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("only superuser or a role with privileges of the pg_execute_server_program role may specify the program option of a file_fdw foreign table")));
+ errmsg("permission denied to set the \"program\" option of a file_fdw foreign table"),
+ errdetail("You must have privileges of the \"%s\" role to set this option.",
+ "pg_execute_server_program")));
filename = defGetString(def);
}
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
index ed97f81dda..5a0bdab326 100644
--- a/contrib/test_decoding/expected/permissions.out
+++ b/contrib/test_decoding/expected/permissions.out
@@ -54,13 +54,16 @@ RESET ROLE;
-- plain user *can't* can control replication
SET ROLE regress_lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege to use replication slots.
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for table lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege to use replication slots.
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege to use replication slots.
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE regress_lr_superuser;
@@ -90,7 +93,8 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
RESET ROLE;
SET ROLE regress_lr_normal;
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege to use replication slots.
RESET ROLE;
-- all users can see existing slots
SET ROLE regress_lr_superuser;
diff --git a/src/backend/backup/basebackup_server.c b/src/backend/backup/basebackup_server.c
index 0258d7a03b..de89df190f 100644
--- a/src/backend/backup/basebackup_server.c
+++ b/src/backend/backup/basebackup_server.c
@@ -72,7 +72,9 @@ bbsink_server_new(bbsink *next, char *pathname)
if (!has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or a role with privileges of the pg_write_server_files role to create backup stored on server")));
+ errmsg("permission denied to create backup stored on server"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_write_server_files")));
CommitTransactionCommand();
/*
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 25c50d66fd..2a39ce0567 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2547,20 +2547,26 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
if (!superuser_arg(roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser")));
+ errmsg("permission denied"),
+ errdetail("You must have %s privilege.",
+ "SUPERUSER")));
}
else
{
if (!has_createrole_privilege(roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have CREATEROLE privilege")));
+ errmsg("permission denied"),
+ errdetail("You must have %s privilege.",
+ "CREATEROLE")));
if (!is_admin_of_role(roleid, address.objectId))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\"",
- GetUserNameFromId(address.objectId,
- true))));
+ errmsg("permission denied"),
+ errdetail("You must have %s on role \"%s\".",
+ "ADMIN OPTION",
+ GetUserNameFromId(address.objectId,
+ true))));
}
break;
case OBJECT_TSPARSER:
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..e557eb17b6 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -83,7 +83,9 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
if (!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_execute_server_program role to COPY to or from an external program"),
+ errmsg("permission denied to COPY to or from an external program"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_execute_server_program"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
}
@@ -92,14 +94,18 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
if (is_from && !has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_read_server_files role to COPY from a file"),
+ errmsg("permission denied to COPY from a file"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_read_server_files"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
if (!is_from && !has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_write_server_files role to COPY to a file"),
+ errmsg("permission denied to COPY to a file"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_write_server_files"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
}
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index f286918f69..699005bd04 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1140,10 +1140,11 @@ CheckSlotRequirements(void)
void
CheckSlotPermissions(void)
{
- if (!superuser() && !has_rolreplication(GetUserId()))
+ if (!has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to use replication slots")));
+ errmsg("permission denied to use replication slots"),
+ errdetail("You must have %s privilege to use replication slots.", "REPLICATION")));
}
/*
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index a7071b2fce..4086938d28 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3860,7 +3860,9 @@ TerminateOtherDBBackends(Oid databaseId)
!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must have privileges of the role whose process is being terminated or have privileges of the \"%s\" role.",
+ "pg_signal_backend")));
}
}
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index bc93ab5b52..7e0f0aef73 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -121,12 +121,16 @@ pg_cancel_backend(PG_FUNCTION_ARGS)
if (r == SIGNAL_BACKEND_NOSUPERUSER)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to cancel superuser query")));
+ errmsg("permission denied to cancel query"),
+ errdetail("You must have %s privilege to cancel queries of roles with %s.",
+ "SUPERUSER", "SUPERUSER")));
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose query is being canceled or member of pg_signal_backend")));
+ errmsg("permission denied to cancel query"),
+ errdetail("You must have privileges of the role whose query is being canceled or have privileges of the \"%s\" role.",
+ "pg_signal_backend")));
PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS);
}
@@ -223,12 +227,16 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
if (r == SIGNAL_BACKEND_NOSUPERUSER)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to terminate superuser process")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must have %s privilege to terminate processes of roles with %s.",
+ "SUPERUSER", "SUPERUSER")));
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must have privileges of the role whose process is being terminated or have privileges of the \"%s\" role.",
+ "pg_signal_backend")));
/* Wait only on success and if actually requested */
if (r == SIGNAL_BACKEND_SUCCESS && timeout > 0)
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c7d9d96b45..0ff9aad106 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -950,7 +950,10 @@ standard_ProcessUtility(PlannedStmt *pstmt,
if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINT))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_checkpoint to do CHECKPOINT")));
+ errmsg("permission denied to execute %s command",
+ "CHECKPOINT"),
+ errdetail("You must have privileges of the \"%s\" role to execute this command.",
+ "pg_checkpoint")));
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
(RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 59532bbd80..b0cfbc742b 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -701,6 +701,10 @@ has_rolreplication(Oid roleid)
bool result = false;
HeapTuple utup;
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
if (HeapTupleIsValid(utup))
{
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2f07ca7a0e..269712215c 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -945,7 +945,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
if (!has_privs_of_role(GetUserId(), ROLE_PG_USE_RESERVED_CONNECTIONS))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("remaining connection slots are reserved for roles with privileges of pg_use_reserved_connections")));
+ errmsg("remaining connection slots are reserved for roles with privileges of \"%s\"",
+ "pg_use_reserved_connections")));
}
/* Check replication permissions needed for walsender processes. */
@@ -953,10 +954,11 @@ InitPostgres(const char *in_dbname, Oid dboid,
{
Assert(!bootstrap);
- if (!superuser() && !has_rolreplication(GetUserId()))
+ if (!has_rolreplication(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to start walsender")));
+ errmsg("permission denied to start WAL sender"),
+ errdetail("You must have %s privilege to start a WAL sender process.", "REPLICATION")));
}
/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index bc83f3577d..d1aa464571 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4208,8 +4208,9 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must have privileges of the \"%s\" role to examine this parameter.",
+ "pg_read_all_settings")));
switch (record->vartype)
{
@@ -4254,8 +4255,9 @@ GetConfigOptionResetString(const char *name)
if (!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must have privileges of the \"%s\" role to examine this parameter.",
+ "pg_read_all_settings")));
switch (record->vartype)
{
@@ -5260,8 +5262,9 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
if (!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must have privileges of the \"%s\" role to examine this parameter.",
+ "pg_read_all_settings")));
if (varname)
*varname = record->name;
diff --git a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
index c57d4fd2df..abb0b2fa0f 100644
--- a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
+++ b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
@@ -59,7 +59,8 @@ SECURITY LABEL ON ROLE regress_dummy_seclabel_user4 IS 'unclassified'; -- fail (
ERROR: role "regress_dummy_seclabel_user4" does not exist
SET SESSION AUTHORIZATION regress_dummy_seclabel_user2;
SECURITY LABEL ON ROLE regress_dummy_seclabel_user2 IS 'unclassified'; -- fail (not privileged)
-ERROR: must have CREATEROLE privilege
+ERROR: permission denied
+DETAIL: You must have CREATEROLE privilege.
RESET SESSION AUTHORIZATION;
--
-- Test for various types of object
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843b..98029cab01 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1077,7 +1077,8 @@ SHOW session_preload_libraries;
SET SESSION AUTHORIZATION regress_role_nopriv;
-- fails with role not member of pg_read_all_settings
SHOW session_preload_libraries;
-ERROR: must be superuser or have privileges of pg_read_all_settings to examine "session_preload_libraries"
+ERROR: permission denied to examine "session_preload_libraries"
+DETAIL: You must have privileges of the "pg_read_all_settings" role to examine this parameter.
RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index 2a7d14dba9..d9620fb8d7 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -106,7 +106,8 @@ ALTER ROLE regress_hasprivs RENAME TO regress_tenant;
ALTER ROLE regress_tenant NOINHERIT NOLOGIN CONNECTION LIMIT 7;
-- fail, we should be unable to modify a role we did not create
COMMENT ON ROLE regress_role_normal IS 'some comment';
-ERROR: must have admin option on role "regress_role_normal"
+ERROR: permission denied
+DETAIL: You must have ADMIN OPTION on role "regress_role_normal".
ALTER ROLE regress_role_normal RENAME TO regress_role_abnormal;
ERROR: permission denied to rename role
DETAIL: You must have CREATEROLE privilege and ADMIN OPTION on role "regress_role_normal".
--
2.25.1
--Qxx1br4bt0+wmkIi--
^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH v4 2/2] Improve more insufficient-privileges error messages.
@ 2023-01-27 22:52 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2023-01-27 22:52 UTC (permalink / raw)
---
contrib/file_fdw/expected/file_fdw.out | 3 ++-
contrib/file_fdw/file_fdw.c | 8 ++++++--
contrib/test_decoding/expected/permissions.out | 12 ++++++++----
src/backend/backup/basebackup_server.c | 4 +++-
src/backend/catalog/objectaddress.c | 16 +++++++++++-----
src/backend/commands/copy.c | 12 +++++++++---
src/backend/replication/slot.c | 5 +++--
src/backend/storage/ipc/procarray.c | 4 +++-
src/backend/storage/ipc/signalfuncs.c | 16 ++++++++++++----
src/backend/tcop/utility.c | 4 +++-
src/backend/utils/init/miscinit.c | 4 ++++
src/backend/utils/init/postinit.c | 8 +++++---
src/backend/utils/misc/guc.c | 15 +++++++++------
.../dummy_seclabel/expected/dummy_seclabel.out | 3 ++-
.../modules/unsafe_tests/expected/rolenames.out | 3 ++-
src/test/regress/expected/create_role.out | 3 ++-
16 files changed, 84 insertions(+), 36 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 36d76ba26c..7e57ad2990 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -474,7 +474,8 @@ ALTER FOREIGN TABLE agg_text OWNER TO regress_file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
SET ROLE regress_file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
-ERROR: only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table
+ERROR: permission denied to specify the "filename" option of a file_fdw foreign table
+DETAIL: You must have privileges of the "pg_read_server_files" role.
SET ROLE regress_file_fdw_superuser;
-- cleanup
RESET ROLE;
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 8ccc167548..696d9e5718 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -278,13 +278,17 @@ file_fdw_validator(PG_FUNCTION_ARGS)
!has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table")));
+ errmsg("permission denied to specify the \"filename\" option of a file_fdw foreign table"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_read_server_files")));
if (strcmp(def->defname, "program") == 0 &&
!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("only superuser or a role with privileges of the pg_execute_server_program role may specify the program option of a file_fdw foreign table")));
+ errmsg("permission denied to specify the \"program\" option of a file_fdw foreign table"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_execute_server_program")));
filename = defGetString(def);
}
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
index ed97f81dda..d102ff066b 100644
--- a/contrib/test_decoding/expected/permissions.out
+++ b/contrib/test_decoding/expected/permissions.out
@@ -54,13 +54,16 @@ RESET ROLE;
-- plain user *can't* can control replication
SET ROLE regress_lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege.
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for table lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege.
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege.
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE regress_lr_superuser;
@@ -90,7 +93,8 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
RESET ROLE;
SET ROLE regress_lr_normal;
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have REPLICATION privilege.
RESET ROLE;
-- all users can see existing slots
SET ROLE regress_lr_superuser;
diff --git a/src/backend/backup/basebackup_server.c b/src/backend/backup/basebackup_server.c
index 0258d7a03b..de89df190f 100644
--- a/src/backend/backup/basebackup_server.c
+++ b/src/backend/backup/basebackup_server.c
@@ -72,7 +72,9 @@ bbsink_server_new(bbsink *next, char *pathname)
if (!has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or a role with privileges of the pg_write_server_files role to create backup stored on server")));
+ errmsg("permission denied to create backup stored on server"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_write_server_files")));
CommitTransactionCommand();
/*
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 25c50d66fd..2a39ce0567 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2547,20 +2547,26 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
if (!superuser_arg(roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser")));
+ errmsg("permission denied"),
+ errdetail("You must have %s privilege.",
+ "SUPERUSER")));
}
else
{
if (!has_createrole_privilege(roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have CREATEROLE privilege")));
+ errmsg("permission denied"),
+ errdetail("You must have %s privilege.",
+ "CREATEROLE")));
if (!is_admin_of_role(roleid, address.objectId))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\"",
- GetUserNameFromId(address.objectId,
- true))));
+ errmsg("permission denied"),
+ errdetail("You must have %s on role \"%s\".",
+ "ADMIN OPTION",
+ GetUserNameFromId(address.objectId,
+ true))));
}
break;
case OBJECT_TSPARSER:
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..e557eb17b6 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -83,7 +83,9 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
if (!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_execute_server_program role to COPY to or from an external program"),
+ errmsg("permission denied to COPY to or from an external program"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_execute_server_program"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
}
@@ -92,14 +94,18 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
if (is_from && !has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_read_server_files role to COPY from a file"),
+ errmsg("permission denied to COPY from a file"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_read_server_files"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
if (!is_from && !has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_write_server_files role to COPY to a file"),
+ errmsg("permission denied to COPY to a file"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_write_server_files"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
}
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index f286918f69..e4df06a05d 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1140,10 +1140,11 @@ CheckSlotRequirements(void)
void
CheckSlotPermissions(void)
{
- if (!superuser() && !has_rolreplication(GetUserId()))
+ if (!has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to use replication slots")));
+ errmsg("permission denied to use replication slots"),
+ errdetail("You must have %s privilege.", "REPLICATION")));
}
/*
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 4340bf9641..95967f8dfa 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3860,7 +3860,9 @@ TerminateOtherDBBackends(Oid databaseId)
!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must have privileges of the role whose process is being terminated or have privileges of the \"%s\" role.",
+ "pg_signal_backend")));
}
}
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index bc93ab5b52..7e0f0aef73 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -121,12 +121,16 @@ pg_cancel_backend(PG_FUNCTION_ARGS)
if (r == SIGNAL_BACKEND_NOSUPERUSER)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to cancel superuser query")));
+ errmsg("permission denied to cancel query"),
+ errdetail("You must have %s privilege to cancel queries of roles with %s.",
+ "SUPERUSER", "SUPERUSER")));
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose query is being canceled or member of pg_signal_backend")));
+ errmsg("permission denied to cancel query"),
+ errdetail("You must have privileges of the role whose query is being canceled or have privileges of the \"%s\" role.",
+ "pg_signal_backend")));
PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS);
}
@@ -223,12 +227,16 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
if (r == SIGNAL_BACKEND_NOSUPERUSER)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to terminate superuser process")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must have %s privilege to terminate processes of roles with %s.",
+ "SUPERUSER", "SUPERUSER")));
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must have privileges of the role whose process is being terminated or have privileges of the \"%s\" role.",
+ "pg_signal_backend")));
/* Wait only on success and if actually requested */
if (r == SIGNAL_BACKEND_SUCCESS && timeout > 0)
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c7d9d96b45..fe77f260c5 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -950,7 +950,9 @@ standard_ProcessUtility(PlannedStmt *pstmt,
if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINT))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_checkpoint to do CHECKPOINT")));
+ errmsg("permission denied to do CHECKPOINT"),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_checkpoint")));
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
(RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 0cdc1e11a3..3a98fafaeb 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -701,6 +701,10 @@ has_rolreplication(Oid roleid)
bool result = false;
HeapTuple utup;
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
if (HeapTupleIsValid(utup))
{
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2f07ca7a0e..808c24b480 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -945,7 +945,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
if (!has_privs_of_role(GetUserId(), ROLE_PG_USE_RESERVED_CONNECTIONS))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("remaining connection slots are reserved for roles with privileges of pg_use_reserved_connections")));
+ errmsg("remaining connection slots are reserved for roles with privileges of \"%s\"",
+ "pg_use_reserved_connections")));
}
/* Check replication permissions needed for walsender processes. */
@@ -953,10 +954,11 @@ InitPostgres(const char *in_dbname, Oid dboid,
{
Assert(!bootstrap);
- if (!superuser() && !has_rolreplication(GetUserId()))
+ if (!has_rolreplication(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to start walsender")));
+ errmsg("permission denied to start walsender"),
+ errdetail("You must have %s privilege.", "REPLICATION")));
}
/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 978b385568..07d97ad75e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4190,8 +4190,9 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_read_all_settings")));
switch (record->vartype)
{
@@ -4236,8 +4237,9 @@ GetConfigOptionResetString(const char *name)
if (!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_read_all_settings")));
switch (record->vartype)
{
@@ -5242,8 +5244,9 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
if (!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must have privileges of the \"%s\" role.",
+ "pg_read_all_settings")));
if (varname)
*varname = record->name;
diff --git a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
index c57d4fd2df..abb0b2fa0f 100644
--- a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
+++ b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
@@ -59,7 +59,8 @@ SECURITY LABEL ON ROLE regress_dummy_seclabel_user4 IS 'unclassified'; -- fail (
ERROR: role "regress_dummy_seclabel_user4" does not exist
SET SESSION AUTHORIZATION regress_dummy_seclabel_user2;
SECURITY LABEL ON ROLE regress_dummy_seclabel_user2 IS 'unclassified'; -- fail (not privileged)
-ERROR: must have CREATEROLE privilege
+ERROR: permission denied
+DETAIL: You must have CREATEROLE privilege.
RESET SESSION AUTHORIZATION;
--
-- Test for various types of object
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843b..6f1fc210a7 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1077,7 +1077,8 @@ SHOW session_preload_libraries;
SET SESSION AUTHORIZATION regress_role_nopriv;
-- fails with role not member of pg_read_all_settings
SHOW session_preload_libraries;
-ERROR: must be superuser or have privileges of pg_read_all_settings to examine "session_preload_libraries"
+ERROR: permission denied to examine "session_preload_libraries"
+DETAIL: You must have privileges of the "pg_read_all_settings" role.
RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index 2a7d14dba9..d9620fb8d7 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -106,7 +106,8 @@ ALTER ROLE regress_hasprivs RENAME TO regress_tenant;
ALTER ROLE regress_tenant NOINHERIT NOLOGIN CONNECTION LIMIT 7;
-- fail, we should be unable to modify a role we did not create
COMMENT ON ROLE regress_role_normal IS 'some comment';
-ERROR: must have admin option on role "regress_role_normal"
+ERROR: permission denied
+DETAIL: You must have ADMIN OPTION on role "regress_role_normal".
ALTER ROLE regress_role_normal RENAME TO regress_role_abnormal;
ERROR: permission denied to rename role
DETAIL: You must have CREATEROLE privilege and ADMIN OPTION on role "regress_role_normal".
--
2.25.1
--azLHFNyN32YCQGCU--
^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH v6 2/2] Improve more insufficient-privileges error messages.
@ 2023-01-27 22:52 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2023-01-27 22:52 UTC (permalink / raw)
---
contrib/file_fdw/expected/file_fdw.out | 3 ++-
contrib/file_fdw/file_fdw.c | 8 ++++++--
contrib/test_decoding/expected/permissions.out | 12 ++++++++----
src/backend/backup/basebackup_server.c | 4 +++-
src/backend/catalog/objectaddress.c | 16 +++++++++++-----
src/backend/commands/copy.c | 12 +++++++++---
src/backend/replication/slot.c | 6 ++++--
src/backend/storage/ipc/procarray.c | 4 +++-
src/backend/storage/ipc/signalfuncs.c | 16 ++++++++++++----
src/backend/tcop/utility.c | 5 ++++-
src/backend/utils/init/miscinit.c | 4 ++++
src/backend/utils/init/postinit.c | 12 ++++++++----
src/backend/utils/misc/guc.c | 15 +++++++++------
.../dummy_seclabel/expected/dummy_seclabel.out | 3 ++-
.../modules/unsafe_tests/expected/rolenames.out | 3 ++-
src/test/regress/expected/create_role.out | 3 ++-
16 files changed, 89 insertions(+), 37 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 36d76ba26c..ea7afd6fe8 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -474,7 +474,8 @@ ALTER FOREIGN TABLE agg_text OWNER TO regress_file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
SET ROLE regress_file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
-ERROR: only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table
+ERROR: permission denied to set the "filename" option of a file_fdw foreign table
+DETAIL: You must inherit privileges of the "pg_read_server_files" role to set this option.
SET ROLE regress_file_fdw_superuser;
-- cleanup
RESET ROLE;
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 8ccc167548..8553d93b3f 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -278,13 +278,17 @@ file_fdw_validator(PG_FUNCTION_ARGS)
!has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table")));
+ errmsg("permission denied to set the \"filename\" option of a file_fdw foreign table"),
+ errdetail("You must inherit privileges of the \"%s\" role to set this option.",
+ "pg_read_server_files")));
if (strcmp(def->defname, "program") == 0 &&
!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("only superuser or a role with privileges of the pg_execute_server_program role may specify the program option of a file_fdw foreign table")));
+ errmsg("permission denied to set the \"program\" option of a file_fdw foreign table"),
+ errdetail("You must inherit privileges of the \"%s\" role to set this option.",
+ "pg_execute_server_program")));
filename = defGetString(def);
}
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
index ed97f81dda..b45f5cf0df 100644
--- a/contrib/test_decoding/expected/permissions.out
+++ b/contrib/test_decoding/expected/permissions.out
@@ -54,13 +54,16 @@ RESET ROLE;
-- plain user *can't* can control replication
SET ROLE regress_lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have the REPLICATION attribute to use replication slots.
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for table lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have the REPLICATION attribute to use replication slots.
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have the REPLICATION attribute to use replication slots.
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE regress_lr_superuser;
@@ -90,7 +93,8 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
RESET ROLE;
SET ROLE regress_lr_normal;
SELECT pg_drop_replication_slot('regression_slot');
-ERROR: must be superuser or replication role to use replication slots
+ERROR: permission denied to use replication slots
+DETAIL: You must have the REPLICATION attribute to use replication slots.
RESET ROLE;
-- all users can see existing slots
SET ROLE regress_lr_superuser;
diff --git a/src/backend/backup/basebackup_server.c b/src/backend/backup/basebackup_server.c
index 0258d7a03b..aac64408d1 100644
--- a/src/backend/backup/basebackup_server.c
+++ b/src/backend/backup/basebackup_server.c
@@ -72,7 +72,9 @@ bbsink_server_new(bbsink *next, char *pathname)
if (!has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or a role with privileges of the pg_write_server_files role to create backup stored on server")));
+ errmsg("permission denied to create backup stored on server"),
+ errdetail("You must inherit privileges of the \"%s\" role.",
+ "pg_write_server_files")));
CommitTransactionCommand();
/*
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 2f688166e1..c1ab4ea161 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2547,20 +2547,26 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
if (!superuser_arg(roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser")));
+ errmsg("permission denied"),
+ errdetail("You must have the %s attribute.",
+ "SUPERUSER")));
}
else
{
if (!has_createrole_privilege(roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have CREATEROLE privilege")));
+ errmsg("permission denied"),
+ errdetail("You must have the %s attribute.",
+ "CREATEROLE")));
if (!is_admin_of_role(roleid, address.objectId))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\"",
- GetUserNameFromId(address.objectId,
- true))));
+ errmsg("permission denied"),
+ errdetail("You must have the %s option on role \"%s\".",
+ "ADMIN",
+ GetUserNameFromId(address.objectId,
+ true))));
}
break;
case OBJECT_TSPARSER:
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..388cfd7cae 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -83,7 +83,9 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
if (!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_execute_server_program role to COPY to or from an external program"),
+ errmsg("permission denied to COPY to or from an external program"),
+ errdetail("You must inherit privileges of the \"%s\" role.",
+ "pg_execute_server_program"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
}
@@ -92,14 +94,18 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
if (is_from && !has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_read_server_files role to COPY from a file"),
+ errmsg("permission denied to COPY from a file"),
+ errdetail("You must inherit privileges of the \"%s\" role.",
+ "pg_read_server_files"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
if (!is_from && !has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of the pg_write_server_files role to COPY to a file"),
+ errmsg("permission denied to COPY to a file"),
+ errdetail("You must inherit privileges of the \"%s\" role.",
+ "pg_write_server_files"),
errhint("Anyone can COPY to stdout or from stdin. "
"psql's \\copy command also works for anyone.")));
}
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index f286918f69..51adcd8418 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1140,10 +1140,12 @@ CheckSlotRequirements(void)
void
CheckSlotPermissions(void)
{
- if (!superuser() && !has_rolreplication(GetUserId()))
+ if (!has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to use replication slots")));
+ errmsg("permission denied to use replication slots"),
+ errdetail("You must have the %s attribute to use replication slots.",
+ "REPLICATION")));
}
/*
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index a7071b2fce..09a69f2c76 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3860,7 +3860,9 @@ TerminateOtherDBBackends(Oid databaseId)
!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must inherit privileges of the role whose process is being terminated or inherit privileges of the \"%s\" role.",
+ "pg_signal_backend")));
}
}
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index bc93ab5b52..c1be67e7d0 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -121,12 +121,16 @@ pg_cancel_backend(PG_FUNCTION_ARGS)
if (r == SIGNAL_BACKEND_NOSUPERUSER)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to cancel superuser query")));
+ errmsg("permission denied to cancel query"),
+ errdetail("You must have the %s attribute to cancel queries of roles with %s.",
+ "SUPERUSER", "SUPERUSER")));
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose query is being canceled or member of pg_signal_backend")));
+ errmsg("permission denied to cancel query"),
+ errdetail("You must inherit privileges of the role whose query is being canceled or inherit privileges of the \"%s\" role.",
+ "pg_signal_backend")));
PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS);
}
@@ -223,12 +227,16 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
if (r == SIGNAL_BACKEND_NOSUPERUSER)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to terminate superuser process")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must have the %s attribute to terminate processes of roles with %s.",
+ "SUPERUSER", "SUPERUSER")));
if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend")));
+ errmsg("permission denied to terminate process"),
+ errdetail("You must inherit privileges of the role whose process is being terminated or inherit privileges of the \"%s\" role.",
+ "pg_signal_backend")));
/* Wait only on success and if actually requested */
if (r == SIGNAL_BACKEND_SUCCESS && timeout > 0)
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c7d9d96b45..3fd9cfb842 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -950,7 +950,10 @@ standard_ProcessUtility(PlannedStmt *pstmt,
if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINT))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_checkpoint to do CHECKPOINT")));
+ errmsg("permission denied to execute %s command",
+ "CHECKPOINT"),
+ errdetail("You must inherit privileges of the \"%s\" role to execute this command.",
+ "pg_checkpoint")));
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
(RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 59532bbd80..b0cfbc742b 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -701,6 +701,10 @@ has_rolreplication(Oid roleid)
bool result = false;
HeapTuple utup;
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
if (HeapTupleIsValid(utup))
{
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2f07ca7a0e..2b103586ab 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -940,12 +940,14 @@ InitPostgres(const char *in_dbname, Oid dboid,
if (nfree < SuperuserReservedConnections)
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("remaining connection slots are reserved for superusers")));
+ errmsg("remaining connection slots are reserved for roles with %s",
+ "SUPERUSER")));
if (!has_privs_of_role(GetUserId(), ROLE_PG_USE_RESERVED_CONNECTIONS))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("remaining connection slots are reserved for roles with privileges of pg_use_reserved_connections")));
+ errmsg("remaining connection slots are reserved for roles that inherit privileges of the \"%s\" role",
+ "pg_use_reserved_connections")));
}
/* Check replication permissions needed for walsender processes. */
@@ -953,10 +955,12 @@ InitPostgres(const char *in_dbname, Oid dboid,
{
Assert(!bootstrap);
- if (!superuser() && !has_rolreplication(GetUserId()))
+ if (!has_rolreplication(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to start walsender")));
+ errmsg("permission denied to start WAL sender"),
+ errdetail("You must have the %s attribute to start a WAL sender process.",
+ "REPLICATION")));
}
/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 51e07d5582..7805a6015b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4209,8 +4209,9 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must inherit privileges of the \"%s\" role to examine this parameter.",
+ "pg_read_all_settings")));
switch (record->vartype)
{
@@ -4255,8 +4256,9 @@ GetConfigOptionResetString(const char *name)
if (!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must inherit privileges of the \"%s\" role to examine this parameter.",
+ "pg_read_all_settings")));
switch (record->vartype)
{
@@ -5261,8 +5263,9 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
if (!ConfigOptionIsVisible(record))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
- name)));
+ errmsg("permission denied to examine \"%s\"", name),
+ errdetail("You must inherit privileges of the \"%s\" role to examine this parameter.",
+ "pg_read_all_settings")));
if (varname)
*varname = record->name;
diff --git a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
index c57d4fd2df..18f8db0e39 100644
--- a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
+++ b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
@@ -59,7 +59,8 @@ SECURITY LABEL ON ROLE regress_dummy_seclabel_user4 IS 'unclassified'; -- fail (
ERROR: role "regress_dummy_seclabel_user4" does not exist
SET SESSION AUTHORIZATION regress_dummy_seclabel_user2;
SECURITY LABEL ON ROLE regress_dummy_seclabel_user2 IS 'unclassified'; -- fail (not privileged)
-ERROR: must have CREATEROLE privilege
+ERROR: permission denied
+DETAIL: You must have the CREATEROLE attribute.
RESET SESSION AUTHORIZATION;
--
-- Test for various types of object
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843b..6e55850705 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1077,7 +1077,8 @@ SHOW session_preload_libraries;
SET SESSION AUTHORIZATION regress_role_nopriv;
-- fails with role not member of pg_read_all_settings
SHOW session_preload_libraries;
-ERROR: must be superuser or have privileges of pg_read_all_settings to examine "session_preload_libraries"
+ERROR: permission denied to examine "session_preload_libraries"
+DETAIL: You must inherit privileges of the "pg_read_all_settings" role to examine this parameter.
RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index 7157d4c589..e72eb91624 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -106,7 +106,8 @@ ALTER ROLE regress_hasprivs RENAME TO regress_tenant;
ALTER ROLE regress_tenant NOINHERIT NOLOGIN CONNECTION LIMIT 7;
-- fail, we should be unable to modify a role we did not create
COMMENT ON ROLE regress_role_normal IS 'some comment';
-ERROR: must have admin option on role "regress_role_normal"
+ERROR: permission denied
+DETAIL: You must have the ADMIN option on role "regress_role_normal".
ALTER ROLE regress_role_normal RENAME TO regress_role_abnormal;
ERROR: permission denied to rename role
DETAIL: You must have the CREATEROLE attribute and the ADMIN option on role "regress_role_normal".
--
2.25.1
--CE+1k2dSO48ffgeK--
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2024-03-12 09:19 Alvaro Herrera <[email protected]>
0 siblings, 2 replies; 10+ messages in thread
From: Alvaro Herrera @ 2024-03-12 09:19 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Denis Laxalde <[email protected]>; vignesh C <[email protected]>; Thomas Munro <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; Gregory Stark (as CFM) <[email protected]>; Jelte Fennema <[email protected]>; Daniel Gustafsson <[email protected]>; Peter Eisentraut <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Robert Haas <[email protected]>; [email protected] <[email protected]>
Here's a last one for the cfbot.
I have a question about this one
int
PQcancelStart(PGcancelConn *cancelConn)
{
[...]
if (cancelConn->conn.status != CONNECTION_ALLOCATED)
{
libpq_append_conn_error(&cancelConn->conn,
"cancel request is already being sent on this connection");
cancelConn->conn.status = CONNECTION_BAD;
return 0;
}
If we do this and we see conn.status is not ALLOCATED, meaning a cancel
is already ongoing, shouldn't we leave conn.status alone instead of
changing to CONNECTION_BAD? I mean, we shouldn't be juggling the elbow
of whoever's doing that, should we? Maybe just add the error message
and return 0?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)
Attachments:
[text/x-diff] v34-0001-libpq-Add-encrypted-and-non-blocking-query-cance.patch (48.2K, ../../[email protected]/2-v34-0001-libpq-Add-encrypted-and-non-blocking-query-cance.patch)
download | inline diff:
From fc0cbf0a6184d374e12e051f88f9f8eef7cc30d9 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email protected]>
Date: Tue, 12 Mar 2024 10:09:25 +0100
Subject: [PATCH v34 1/2] libpq: Add encrypted and non-blocking query
cancellation routines
The existing PQcancel API uses blocking IO, which makes PQcancel
impossible to use in an event loop based codebase without blocking the
event loop until the call returns. It also doesn't encrypt the
connection over which the cancel request is sent, even when the original
connection required encryption.
This commit adds a PQcancelConn struct and assorted functions, which
provide a better mechanism of sending cancel requests; in particular all
the encryption used in the original connection are also used in the
cancel connection. The main entry points are:
- PQcancelCreate creates the PQcancelConn based on the original
connection (but does not establish an actual connection).
- PQcancelStart can be used to initiate non-blocking cancel requests,
using encryption if the original connection did so, which must be
pumped using
- PQcancelPoll.
- PQcancelReset puts a PQcancelConn back in state so that it can be
reused to send a new cancel request to the same connection.
- PQcancelBlocking is a simpler-to-use blocking API that still uses
encryption.
Additional functions are
- PQcancelStatus, mimicks PQstatus;
- PQcancelSocket, mimicks PQcancelSocket;
- PQcancelErrorMessage, mimicks PQerrorMessage;
- PQcancelFinish, mimicks PQfinish.
Author: Jelte Fennema-Nio <[email protected]>
Reviewed-by: Denis Laxalde <[email protected]>
Discussion: https://postgr.es/m/AM5PR83MB0178D3B31CA1B6EC4A8ECC42F7529@AM5PR83MB0178.EURPRD83.prod.outlook.com
---
doc/src/sgml/libpq.sgml | 465 ++++++++++++++++--
src/interfaces/libpq/exports.txt | 9 +
src/interfaces/libpq/fe-cancel.c | 297 ++++++++++-
src/interfaces/libpq/fe-connect.c | 129 ++++-
src/interfaces/libpq/libpq-fe.h | 31 +-
src/interfaces/libpq/libpq-int.h | 5 +
.../modules/libpq_pipeline/libpq_pipeline.c | 125 +++++
src/tools/pgindent/typedefs.list | 1 +
8 files changed, 1015 insertions(+), 47 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index a2bbf33d02..373d0dc322 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -265,7 +265,7 @@ PGconn *PQsetdb(char *pghost,
<varlistentry id="libpq-PQconnectStartParams">
<term><function>PQconnectStartParams</function><indexterm><primary>PQconnectStartParams</primary></indexterm></term>
<term><function>PQconnectStart</function><indexterm><primary>PQconnectStart</primary></indexterm></term>
- <term><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
+ <term id="libpq-PQconnectPoll"><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
<listitem>
<para>
<indexterm><primary>nonblocking connection</primary></indexterm>
@@ -5287,7 +5287,7 @@ int PQisBusy(PGconn *conn);
<xref linkend="libpq-PQsendQuery"/>/<xref linkend="libpq-PQgetResult"/>
can also attempt to cancel a command that is still being processed
by the server; see <xref linkend="libpq-cancel"/>. But regardless of
- the return value of <xref linkend="libpq-PQcancel"/>, the application
+ the return value of <xref linkend="libpq-PQcancelBlocking"/>, the application
must continue with the normal result-reading sequence using
<xref linkend="libpq-PQgetResult"/>. A successful cancellation will
simply cause the command to terminate sooner than it would have
@@ -6034,10 +6034,402 @@ int PQsetSingleRowMode(PGconn *conn);
<secondary>SQL command</secondary>
</indexterm>
- <para>
- A client application can request cancellation of a command that is
- still being processed by the server, using the functions described in
- this section.
+ <sect2 id="libpq-cancel-conn">
+ <title>Functions for Sending Cancel Requests</title>
+ <variablelist>
+ <varlistentry id="libpq-PQcancelCreate">
+ <term><function>PQcancelCreate</function><indexterm><primary>PQcancelCreate</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Prepares a connection over which a cancel request can be sent.
+<synopsis>
+PGcancelConn *PQcancelCreate(PGconn *conn);
+</synopsis>
+ </para>
+
+ <para>
+ <xref linkend="libpq-PQcancelCreate"/> creates a
+ <structname>PGcancelConn</structname><indexterm><primary>PGcancelConn</primary></indexterm>
+ object, but it won't instantly start sending a cancel request over this
+ connection. A cancel request can be sent over this connection in a
+ blocking manner using <xref linkend="libpq-PQcancelBlocking"/> and in a
+ non-blocking manner using <xref linkend="libpq-PQcancelStart"/>.
+ The return value can be passed to <xref linkend="libpq-PQcancelStatus"/>
+ to check if the <structname>PGcancelConn</structname> object was
+ created successfully. The <structname>PGcancelConn</structname> object
+ is an opaque structure that is not meant to be accessed directly by the
+ application. This <structname>PGcancelConn</structname> object can be
+ used to cancel the query that's running on the original connection in a
+ thread-safe way.
+ </para>
+
+ <para>
+ Many connection parameters of the original client will be reused when
+ setting up the connection for the cancel request. Importantly, if the
+ original connection requires encryption of the connection and/or
+ verification of the target host (using <literal>sslmode</literal> or
+ <literal>gssencmode</literal>), then the connection for the cancel
+ request is made with these same requirements. Any connection options
+ that are only used during authentication or after authentication of the
+ client are ignored though, because cancellation requests do not require
+ authentication and the connection is closed right after the cancellation
+ request is submitted.
+ </para>
+
+ <para>
+ Note that when <function>PQcancelCreate</function> returns a non-null
+ pointer, you must call <xref linkend="libpq-PQcancelFinish"/> when you
+ are finished with it, in order to dispose of the structure and any
+ associated memory blocks. This must be done even if the cancel request
+ failed or was abandoned.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelBlocking">
+ <term><function>PQcancelBlocking</function><indexterm><primary>PQcancelBlocking</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Requests that the server abandons processing of the current command in a blocking manner.
+<synopsis>
+int PQcancelBlocking(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The request is made over the given <structname>PGcancelConn</structname>,
+ which needs to be created with <xref linkend="libpq-PQcancelCreate"/>.
+ The return value of <xref linkend="libpq-PQcancelBlocking"/>
+ is 1 if the cancel request was successfully
+ dispatched and 0 if not. If it was unsuccessful, the error message can be
+ retrieved using <xref linkend="libpq-PQcancelErrorMessage"/>.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelStart">
+ <term><function>PQcancelStart</function><indexterm><primary>PQcancelStart</primary></indexterm></term>
+ <term id="libpq-PQcancelPoll"><function>PQcancelPoll</function><indexterm><primary>PQcancelPoll</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Requests that the server abandons processing of the current command in a non-blocking manner.
+<synopsis>
+int PQcancelStart(PGcancelConn *cancelConn);
+
+PostgresPollingStatusType PQcancelPoll(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The request is made over the given <structname>PGcancelConn</structname>,
+ which needs to be created with <xref linkend="libpq-PQcancelCreate"/>.
+ The return value of <xref linkend="libpq-PQcancelStart"/>
+ is 1 if the cancellation request could be started and 0 if not.
+ If it was unsuccessful, the error message can be
+ retrieved using <xref linkend="libpq-PQcancelErrorMessage"/>.
+ </para>
+
+ <para>
+ If <function>PQcancelStart</function> succeeds, the next stage
+ is to poll <application>libpq</application> so that it can proceed with
+ the cancel connection sequence.
+ Use <xref linkend="libpq-PQcancelSocket"/> to obtain the descriptor of the
+ socket underlying the database connection.
+ (Caution: do not assume that the socket remains the same
+ across <function>PQcancelPoll</function> calls.)
+ Loop thus: If <function>PQcancelPoll(cancelConn)</function> last returned
+ <symbol>PGRES_POLLING_READING</symbol>, wait until the socket is ready to
+ read (as indicated by <function>select()</function>, <function>poll()</function>, or
+ similar system function).
+ Then call <function>PQcancelPoll(cancelConn)</function> again.
+ Conversely, if <function>PQcancelPoll(cancelConn)</function> last returned
+ <symbol>PGRES_POLLING_WRITING</symbol>, wait until the socket is ready
+ to write, then call <function>PQcancelPoll(cancelConn)</function> again.
+ On the first iteration, i.e., if you have yet to call
+ <function>PQcancelPoll(cancelConn)</function>, behave as if it last returned
+ <symbol>PGRES_POLLING_WRITING</symbol>. Continue this loop until
+ <function>PQcancelPoll(cancelConn)</function> returns
+ <symbol>PGRES_POLLING_FAILED</symbol>, indicating the connection procedure
+ has failed, or <symbol>PGRES_POLLING_OK</symbol>, indicating cancel
+ request was successfully dispatched.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ <para>
+ At any time during connection, the status of the connection can be
+ checked by calling <xref linkend="libpq-PQcancelStatus"/>. If this call returns <symbol>CONNECTION_BAD</symbol>, then the
+ cancel procedure has failed; if the call returns <function>CONNECTION_OK</function>, then cancel request was successfully dispatched. Both of these states are equally detectable
+ from the return value of <function>PQcancelPoll</function>, described above. Other states might also occur
+ during (and only during) an asynchronous connection procedure. These
+ indicate the current stage of the connection procedure and might be useful
+ to provide feedback to the user for example. These statuses are:
+
+ <variablelist>
+ <varlistentry id="libpq-connection-allocated">
+ <term><symbol>CONNECTION_ALLOCATED</symbol></term>
+ <listitem>
+ <para>
+ Waiting for a call to <xref linkend="libpq-PQcancelStart"/> or
+ <xref linkend="libpq-PQcancelBlocking"/>, to actually open the
+ socket. This is the connection state right after
+ calling <xref linkend="libpq-PQcancelCreate"/>
+ or <xref linkend="libpq-PQcancelReset"/>. No connection to the
+ server has been initiated yet at this point. To actually start
+ sending the cancel request use <xref linkend="libpq-PQcancelStart"/> or
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-started">
+ <term><symbol>CONNECTION_STARTED</symbol></term>
+ <listitem>
+ <para>
+ Waiting for connection to be made.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-made">
+ <term><symbol>CONNECTION_MADE</symbol></term>
+ <listitem>
+ <para>
+ Connection OK; waiting to send.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-awaiting-response">
+ <term><symbol>CONNECTION_AWAITING_RESPONSE</symbol></term>
+ <listitem>
+ <para>
+ Waiting for a response from the server.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-ssl-startup">
+ <term><symbol>CONNECTION_SSL_STARTUP</symbol></term>
+ <listitem>
+ <para>
+ Negotiating SSL encryption.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-gss-startup">
+ <term><symbol>CONNECTION_GSS_STARTUP</symbol></term>
+ <listitem>
+ <para>
+ Negotiating GSS encryption.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ Note that, although these constants will remain (in order to maintain
+ compatibility), an application should never rely upon these occurring in a
+ particular order, or at all, or on the status always being one of these
+ documented values. An application might do something like this:
+<programlisting>
+switch(PQcancelStatus(conn))
+{
+ case CONNECTION_STARTED:
+ feedback = "Connecting...";
+ break;
+
+ case CONNECTION_MADE:
+ feedback = "Connected to server...";
+ break;
+.
+.
+.
+ default:
+ feedback = "Connecting...";
+}
+</programlisting>
+ </para>
+
+ <para>
+ The <literal>connect_timeout</literal> connection parameter is ignored
+ when using <function>PQcancelPoll</function>; it is the application's
+ responsibility to decide whether an excessive amount of time has elapsed.
+ Otherwise, <function>PQcancelStart</function> followed by a
+ <function>PQcancelPoll</function> loop is equivalent to
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelStatus">
+ <term><function>PQcancelStatus</function><indexterm><primary>PQcancelStatus</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Returns the status of the cancel connection.
+<synopsis>
+ConnStatusType PQcancelStatus(const PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The status can be one of a number of values. However, only three of
+ these are seen outside of an asynchronous cancel procedure:
+ <literal>CONNECTION_ALLOCATED</literal>,
+ <literal>CONNECTION_OK</literal> and
+ <literal>CONNECTION_BAD</literal>. The initial state of a
+ <function>PGcancelConn</function> that's successfully created using
+ <xref linkend="libpq-PQcancelCreate"/> is <literal>CONNECTION_ALLOCATED</literal>.
+ A cancel request that was successfully dispatched
+ has the status <literal>CONNECTION_OK</literal>. A failed
+ cancel attempt is signaled by status
+ <literal>CONNECTION_BAD</literal>. An OK status will
+ remain so until <xref linkend="libpq-PQcancelFinish"/> or
+ <xref linkend="libpq-PQcancelReset"/> is called.
+ </para>
+
+ <para>
+ See the entry for <xref linkend="libpq-PQcancelStart"/> and <xref
+ linkend="libpq-PQcancelPoll"/> with regards to other status codes that
+ might be returned.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelSocket">
+ <term><function>PQcancelSocket</function><indexterm><primary>PQcancelSocket</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Obtains the file descriptor number of the cancel connection socket to
+ the server. A valid descriptor will be greater than or equal
+ to 0; a result of -1 indicates that no server connection is
+ currently open. This might change as a result of calling all of the
+ functions in this section on the (except for
+ <xref linkend="libpq-PQcancelErrorMessage"/> and
+ <function>PQcancelSocket</function> itself).
+<synopsis>
+int PQcancelSocket(const PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelErrorMessage">
+ <term><function>PQcancelErrorMessage</function><indexterm><primary>PQcancelErrorMessage</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ <indexterm><primary>error message</primary></indexterm> Returns the error message
+ most recently generated by an operation on the cancel connection.
+<synopsis>
+char *PQcancelErrorMessage(const PGcancelConn *cancelconn);
+</synopsis>
+ </para>
+
+ <para>
+ Nearly all <application>libpq</application> functions that take a
+ <structname>PGcancelConn</structname> will set a message for
+ <xref linkend="libpq-PQcancelErrorMessage"/> if they fail. Note that by
+ <application>libpq</application> convention, a nonempty
+ <xref linkend="libpq-PQcancelErrorMessage"/> result can consist of multiple lines,
+ and will include a trailing newline. The caller should not free
+ the result directly. It will be freed when the associated
+ <structname>PGcancelConn</structname> handle is passed to
+ <xref linkend="libpq-PQcancelFinish"/>. The result string should not be
+ expected to remain the same across operations on the
+ <literal>PGcancelConn</literal> structure.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelFinish">
+ <term><function>PQcancelFinish</function><indexterm><primary>PQcancelFinish</primary></indexterm></term>
+ <listitem>
+ <para>
+ Closes the cancel connection (if it did not finish sending the cancel
+ request yet). Also frees memory used by the <structname>PGcancelConn</structname>
+ object.
+<synopsis>
+void PQcancelFinish(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ Note that even if the cancel attempt fails (as
+ indicated by <xref linkend="libpq-PQcancelStatus"/>), the application should call <xref linkend="libpq-PQcancelFinish"/>
+ to free the memory used by the <structname>PGcancelConn</structname> object.
+ The <structname>PGcancelConn</structname> pointer must not be used again after
+ <xref linkend="libpq-PQcancelFinish"/> has been called.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelReset">
+ <term><function>PQcancelReset</function><indexterm><primary>PQcancelReset</primary></indexterm></term>
+ <listitem>
+ <para>
+ Resets the <symbol>PGcancelConn</symbol> so it can be reused for a new
+ cancel connection.
+<synopsis>
+void PQcancelReset(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ If the <symbol>PGcancelConn</symbol> is currently used to send a cancel
+ request, then this connection is closed. It will then prepare the
+ <symbol>PGcancelConn</symbol> object such that it can be used to send a
+ new cancel request. This can be used to create one <symbol>PGcancelConn</symbol>
+ for a <symbol>PGconn</symbol> and reuse that multiple times throughout
+ the lifetime of the original <symbol>PGconn</symbol>.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </sect2>
+
+ <sect2 id="libpq-cancel-deprecated">
+ <title>Obsolete Functions for Sending Cancel Requests</title>
+
+ <para>
+ These functions represent older methods of sending cancel requests.
+ Although they still work, they are deprecated due to not sending the cancel
+ requests in an encrypted manner, even when the original connection
+ specified <literal>sslmode</literal> or <literal>gssencmode</literal> to
+ require encryption. Thus these older methods are heavily discouraged from
+ being used in new code, and it is recommended to change existing code to
+ use the new functions instead.
+ </para>
<variablelist>
<varlistentry id="libpq-PQgetCancel">
@@ -6046,7 +6438,7 @@ int PQsetSingleRowMode(PGconn *conn);
<listitem>
<para>
Creates a data structure containing the information needed to cancel
- a command issued through a particular database connection.
+ a command using <xref linkend="libpq-PQcancel"/>.
<synopsis>
PGcancel *PQgetCancel(PGconn *conn);
</synopsis>
@@ -6088,37 +6480,38 @@ void PQfreeCancel(PGcancel *cancel);
<listitem>
<para>
- Requests that the server abandon processing of the current command.
-<synopsis>
+ <xref linkend="libpq-PQrequestCancel"/> is a deprecated and insecure
+ variant of <xref linkend="libpq-PQcancelBlocking"/>, but one that can be
+ used safely from within a signal handler. <synopsis>
int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
</synopsis>
</para>
<para>
- The return value is 1 if the cancel request was successfully
+ <xref linkend="libpq-PQcancel"/> only exists because of backwards
+ compatibility reasons. <xref linkend="libpq-PQcancelBlocking"/> should be
+ used instead. The only benefit that <xref linkend="libpq-PQcancel"/> has
+ is that it can be safely invoked from a signal handler, if the
+ <parameter>errbuf</parameter> is a local variable in the signal handler.
+ However, this is generally not considered a big enough benefit to be
+ worth the security issues that this function has.
+ </para>
+
+ <para>
+ The <structname>PGcancel</structname> object is read-only as far as
+ <xref linkend="libpq-PQcancel"/> is concerned, so it can also be invoked
+ from a thread that is separate from the one manipulating the
+ <structname>PGconn</structname> object.
+ </para>
+
+ <para>
+ The return value of <xref linkend="libpq-PQcancel"/>
+ is 1 if the cancel request was successfully
dispatched and 0 if not. If not, <parameter>errbuf</parameter> is filled
with an explanatory error message. <parameter>errbuf</parameter>
must be a char array of size <parameter>errbufsize</parameter> (the
recommended size is 256 bytes).
</para>
-
- <para>
- Successful dispatch is no guarantee that the request will have
- any effect, however. If the cancellation is effective, the current
- command will terminate early and return an error result. If the
- cancellation fails (say, because the server was already done
- processing the command), then there will be no visible result at
- all.
- </para>
-
- <para>
- <xref linkend="libpq-PQcancel"/> can safely be invoked from a signal
- handler, if the <parameter>errbuf</parameter> is a local variable in the
- signal handler. The <structname>PGcancel</structname> object is read-only
- as far as <xref linkend="libpq-PQcancel"/> is concerned, so it can
- also be invoked from a thread that is separate from the one
- manipulating the <structname>PGconn</structname> object.
- </para>
</listitem>
</varlistentry>
</variablelist>
@@ -6129,13 +6522,21 @@ int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
<listitem>
<para>
- <xref linkend="libpq-PQrequestCancel"/> is a deprecated variant of
- <xref linkend="libpq-PQcancel"/>.
+ <xref linkend="libpq-PQrequestCancel"/> is a deprecated and insecure
+ variant of <xref linkend="libpq-PQcancelBlocking"/>.
<synopsis>
int PQrequestCancel(PGconn *conn);
</synopsis>
</para>
+ <para>
+ <xref linkend="libpq-PQrequestCancel"/> only exists because of backwards
+ compatibility reasons. <xref linkend="libpq-PQcancelBlocking"/> should be
+ used instead. There is no benefit to using
+ <xref linkend="libpq-PQrequestCancel"/> over
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+
<para>
Requests that the server abandon processing of the current
command. It operates directly on the
@@ -6150,7 +6551,7 @@ int PQrequestCancel(PGconn *conn);
</listitem>
</varlistentry>
</variablelist>
- </para>
+ </sect2>
</sect1>
@@ -9362,7 +9763,7 @@ int PQisthreadsafe();
The deprecated functions <xref linkend="libpq-PQrequestCancel"/> and
<xref linkend="libpq-PQoidStatus"/> are not thread-safe and should not be
used in multithread programs. <xref linkend="libpq-PQrequestCancel"/>
- can be replaced by <xref linkend="libpq-PQcancel"/>.
+ can be replaced by <xref linkend="libpq-PQcancelBlocking"/>.
<xref linkend="libpq-PQoidStatus"/> can be replaced by
<xref linkend="libpq-PQoidValue"/>.
</para>
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 088592deb1..9fbd3d3407 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -193,3 +193,12 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQchangePassword 192
PQsendPipelineSync 193
+PQcancelBlocking 194
+PQcancelStart 195
+PQcancelCreate 196
+PQcancelPoll 197
+PQcancelStatus 198
+PQcancelSocket 199
+PQcancelErrorMessage 200
+PQcancelReset 201
+PQcancelFinish 202
diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c
index d69b8f9f9f..5cc674820a 100644
--- a/src/interfaces/libpq/fe-cancel.c
+++ b/src/interfaces/libpq/fe-cancel.c
@@ -22,6 +22,18 @@
#include "port/pg_bswap.h"
+/*
+ * pg_cancel_conn is a wrapper around a PGconn to send cancellations using
+ * PQcancelBlocking and PQcancelStart. This isn't just a typedef because we
+ * want the compiler to complain when a PGconn is passed to a function that
+ * expects a PGcancelConn, and vice versa.
+ */
+struct pg_cancel_conn
+{
+ PGconn conn;
+};
+
+
/*
* pg_cancel (backing struct for PGcancel) stores all data necessary to send a
* cancel request.
@@ -41,6 +53,289 @@ struct pg_cancel
};
+/*
+ * PQcancelCreate
+ *
+ * Create and return a PGcancelConn, which can be used to securely cancel a
+ * query on the given connection.
+ *
+ * This requires either following the non-blocking flow through
+ * PQcancelStart() and PQcancelPoll(), or the blocking PQcancelBlocking().
+ */
+PGcancelConn *
+PQcancelCreate(PGconn *conn)
+{
+ PGconn *cancelConn = pqMakeEmptyPGconn();
+ pg_conn_host originalHost;
+
+ if (cancelConn == NULL)
+ return NULL;
+
+ /* Check we have an open connection */
+ if (!conn)
+ {
+ libpq_append_conn_error(cancelConn, "passed connection was NULL");
+ return (PGcancelConn *) cancelConn;
+ }
+
+ if (conn->sock == PGINVALID_SOCKET)
+ {
+ libpq_append_conn_error(cancelConn, "passed connection is not open");
+ return (PGcancelConn *) cancelConn;
+ }
+
+ /*
+ * Indicate that this connection is used to send a cancellation
+ */
+ cancelConn->cancelRequest = true;
+
+ if (!pqCopyPGconn(conn, cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Compute derived options
+ */
+ if (!pqConnectOptions2(cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Copy cancellation token data from the original connnection
+ */
+ cancelConn->be_pid = conn->be_pid;
+ cancelConn->be_key = conn->be_key;
+
+ /*
+ * Cancel requests should not iterate over all possible hosts. The request
+ * needs to be sent to the exact host and address that the original
+ * connection used. So we manually create the host and address arrays with
+ * a single element after freeing the host array that we generated from
+ * the connection options.
+ */
+ pqReleaseConnHosts(cancelConn);
+ cancelConn->nconnhost = 1;
+ cancelConn->naddr = 1;
+
+ cancelConn->connhost = calloc(cancelConn->nconnhost, sizeof(pg_conn_host));
+ if (!cancelConn->connhost)
+ goto oom_error;
+
+ originalHost = conn->connhost[conn->whichhost];
+ if (originalHost.host)
+ {
+ cancelConn->connhost[0].host = strdup(originalHost.host);
+ if (!cancelConn->connhost[0].host)
+ goto oom_error;
+ }
+ if (originalHost.hostaddr)
+ {
+ cancelConn->connhost[0].hostaddr = strdup(originalHost.hostaddr);
+ if (!cancelConn->connhost[0].hostaddr)
+ goto oom_error;
+ }
+ if (originalHost.port)
+ {
+ cancelConn->connhost[0].port = strdup(originalHost.port);
+ if (!cancelConn->connhost[0].port)
+ goto oom_error;
+ }
+ if (originalHost.password)
+ {
+ cancelConn->connhost[0].password = strdup(originalHost.password);
+ if (!cancelConn->connhost[0].password)
+ goto oom_error;
+ }
+
+ cancelConn->addr = calloc(cancelConn->naddr, sizeof(AddrInfo));
+ if (!cancelConn->connhost)
+ goto oom_error;
+
+ cancelConn->addr[0].addr = conn->raddr;
+ cancelConn->addr[0].family = conn->raddr.addr.ss_family;
+
+ cancelConn->status = CONNECTION_ALLOCATED;
+ return (PGcancelConn *) cancelConn;
+
+oom_error:
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(cancelConn, "out of memory");
+ return (PGcancelConn *) cancelConn;
+}
+
+
+/*
+ * PQcancelBlocking
+ *
+ * Send a cancellation request in a blocking fashion.
+ * Returns 1 if successful 0 if not.
+ */
+int
+PQcancelBlocking(PGcancelConn *cancelConn)
+{
+ if (!PQcancelStart(cancelConn))
+ return 0;
+ return pqConnectDBComplete(&cancelConn->conn);
+}
+
+/*
+ * PQcancelStart
+ *
+ * Starts sending a cancellation request in a non-blocking fashion. Returns
+ * 1 if successful 0 if not.
+ */
+int
+PQcancelStart(PGcancelConn *cancelConn)
+{
+ if (!cancelConn || cancelConn->conn.status == CONNECTION_BAD)
+ return 0;
+
+ if (cancelConn->conn.status != CONNECTION_ALLOCATED)
+ {
+ libpq_append_conn_error(&cancelConn->conn,
+ "cancel request is already being sent on this connection");
+ cancelConn->conn.status = CONNECTION_BAD;
+ return 0;
+ }
+
+ return pqConnectDBStart(&cancelConn->conn);
+}
+
+/*
+ * PQcancelPoll
+ *
+ * Poll a cancel connection. For usage details see PQconnectPoll.
+ */
+PostgresPollingStatusType
+PQcancelPoll(PGcancelConn *cancelConn)
+{
+ PGconn *conn = &cancelConn->conn;
+ int n;
+
+ /*
+ * We leave most of the connection establishement to PQconnectPoll, since
+ * it's very similar to normal connection establishment. But once we get
+ * to the CONNECTION_AWAITING_RESPONSE we need to start doing our own
+ * thing.
+ */
+ if (conn->status != CONNECTION_AWAITING_RESPONSE)
+ {
+ return PQconnectPoll(conn);
+ }
+
+ /*
+ * At this point we are waiting on the server to close the connection,
+ * which is its way of communicating that the cancel has been handled.
+ */
+
+ n = pqReadData(conn);
+
+ if (n == 0)
+ return PGRES_POLLING_READING;
+
+#ifndef WIN32
+
+ /*
+ * If we receive an error report it, but only if errno is non-zero.
+ * Otherwise we assume it's an EOF, which is what we expect from the
+ * server.
+ *
+ * We skip this for Windows, because Windows is a bit special in its EOF
+ * behaviour for TCP. Sometimes it will error with an ECONNRESET when
+ * there is a clean connection closure. See these threads for details:
+ * https://www.postgresql.org/message-id/flat/90b34057-4176-7bb0-0dbb-9822a5f6425b%40greiz-reinsdorf.de
+ *
+ * https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BOeoETZQ%3DQw5Ub5h3tmwQhBmDA%3DnuNO3KG%3DzWfUypFAw%40mail.gmail.com
+ *
+ * PQcancel ignores such errors and reports success for the cancellation
+ * anyway, so even if this is not always correct we do the same here.
+ */
+ if (n < 0 && errno != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ return PGRES_POLLING_FAILED;
+ }
+#endif
+
+ /*
+ * We don't expect any data, only connection closure. So if we strangely
+ * do receive some data we consider that an error.
+ */
+ if (n > 0)
+ {
+ libpq_append_conn_error(conn, "received unexpected response from server");
+ conn->status = CONNECTION_BAD;
+ return PGRES_POLLING_FAILED;
+ }
+
+ /*
+ * Getting here means that we received an EOF, which is what we were
+ * expecting -- the cancel request has completed.
+ */
+ cancelConn->conn.status = CONNECTION_OK;
+ resetPQExpBuffer(&conn->errorMessage);
+ return PGRES_POLLING_OK;
+}
+
+/*
+ * PQcancelStatus
+ *
+ * Get the status of a cancel connection.
+ */
+ConnStatusType
+PQcancelStatus(const PGcancelConn *cancelConn)
+{
+ return PQstatus(&cancelConn->conn);
+}
+
+/*
+ * PQcancelSocket
+ *
+ * Get the socket of the cancel connection.
+ */
+int
+PQcancelSocket(const PGcancelConn *cancelConn)
+{
+ return PQsocket(&cancelConn->conn);
+}
+
+/*
+ * PQcancelErrorMessage
+ *
+ * Get the socket of the cancel connection.
+ */
+char *
+PQcancelErrorMessage(const PGcancelConn *cancelConn)
+{
+ return PQerrorMessage(&cancelConn->conn);
+}
+
+/*
+ * PQcancelReset
+ *
+ * Resets the cancel connection, so it can be reused to send a new cancel
+ * request.
+ */
+void
+PQcancelReset(PGcancelConn *cancelConn)
+{
+ pqClosePGconn(&cancelConn->conn);
+ cancelConn->conn.status = CONNECTION_ALLOCATED;
+ cancelConn->conn.whichhost = 0;
+ cancelConn->conn.whichaddr = 0;
+ cancelConn->conn.try_next_host = false;
+ cancelConn->conn.try_next_addr = false;
+}
+
+/*
+ * PQcancelFinish
+ *
+ * Closes and frees the cancel connection.
+ */
+void
+PQcancelFinish(PGcancelConn *cancelConn)
+{
+ PQfinish(&cancelConn->conn);
+}
+
/*
* PQgetCancel: get a PGcancel structure corresponding to a connection.
*
@@ -145,7 +440,7 @@ optional_setsockopt(int fd, int protoid, int optid, int value)
/*
- * PQcancel: request query cancel
+ * PQcancel: old, non-encrypted, but signal-safe way of requesting query cancel
*
* The return value is true if the cancel request was successfully
* dispatched, false if not (in which case an error message is available).
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d4e10a0c4f..8e8634e5ba 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -616,8 +616,17 @@ pqDropServerData(PGconn *conn)
conn->write_failed = false;
free(conn->write_err_msg);
conn->write_err_msg = NULL;
- conn->be_pid = 0;
- conn->be_key = 0;
+
+ /*
+ * Cancel connections need to retain their be_pid and be_key across
+ * PQcancelReset invocations, otherwise they would not have access to the
+ * secret token of the connection they are supposed to cancel.
+ */
+ if (!conn->cancelRequest)
+ {
+ conn->be_pid = 0;
+ conn->be_key = 0;
+ }
}
@@ -923,6 +932,45 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
return true;
}
+/*
+ * Copy over option values from srcConn to dstConn
+ *
+ * Don't put anything cute here --- intelligence should be in
+ * connectOptions2 ...
+ *
+ * Returns true on success. On failure, returns false and sets error message of
+ * dstConn.
+ */
+bool
+pqCopyPGconn(PGconn *srcConn, PGconn *dstConn)
+{
+ const internalPQconninfoOption *option;
+
+ /* copy over connection options */
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ if (option->connofs >= 0)
+ {
+ const char **tmp = (const char **) ((char *) srcConn + option->connofs);
+
+ if (*tmp)
+ {
+ char **dstConnmember = (char **) ((char *) dstConn + option->connofs);
+
+ if (*dstConnmember)
+ free(*dstConnmember);
+ *dstConnmember = strdup(*tmp);
+ if (*dstConnmember == NULL)
+ {
+ libpq_append_conn_error(dstConn, "out of memory");
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+}
+
/*
* connectOptions1
*
@@ -2308,10 +2356,18 @@ pqConnectDBStart(PGconn *conn)
* Set up to try to connect to the first host. (Setting whichhost = -1 is
* a bit of a cheat, but PQconnectPoll will advance it to 0 before
* anything else looks at it.)
+ *
+ * Cancel requests are special though, they should only try one host and
+ * address, and these fields have already been set up in PQcancelCreate, so
+ * leave these fields alone for cancel requests.
*/
- conn->whichhost = -1;
- conn->try_next_addr = false;
- conn->try_next_host = true;
+ if (!conn->cancelRequest)
+ {
+ conn->whichhost = -1;
+ conn->try_next_host = true;
+ conn->try_next_addr = false;
+ }
+
conn->status = CONNECTION_NEEDED;
/* Also reset the target_server_type state if needed */
@@ -2453,7 +2509,10 @@ pqConnectDBComplete(PGconn *conn)
/*
* Now try to advance the state machine.
*/
- flag = PQconnectPoll(conn);
+ if (conn->cancelRequest)
+ flag = PQcancelPoll((PGcancelConn *) conn);
+ else
+ flag = PQconnectPoll(conn);
}
}
@@ -2578,13 +2637,17 @@ keep_going: /* We will come back to here until there is
* Oops, no more hosts.
*
* If we are trying to connect in "prefer-standby" mode, then drop
- * the standby requirement and start over.
+ * the standby requirement and start over. Don't do this for
+ * cancel requests though, since we are certain the list of
+ * servers won't change as the target_server_type option is not
+ * applicable to those connections.
*
* Otherwise, an appropriate error message is already set up, so
* we just need to set the right status.
*/
if (conn->target_server_type == SERVER_TYPE_PREFER_STANDBY &&
- conn->nconnhost > 0)
+ conn->nconnhost > 0 &&
+ !conn->cancelRequest)
{
conn->target_server_type = SERVER_TYPE_PREFER_STANDBY_PASS2;
conn->whichhost = 0;
@@ -3226,6 +3289,29 @@ keep_going: /* We will come back to here until there is
}
#endif /* USE_SSL */
+ /*
+ * For cancel requests this is as far as we need to go in the
+ * connection establishment. Now we can actually send our
+ * cancellation request.
+ */
+ if (conn->cancelRequest)
+ {
+ CancelRequestPacket cancelpacket;
+
+ packetlen = sizeof(cancelpacket);
+ cancelpacket.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
+ cancelpacket.backendPID = pg_hton32(conn->be_pid);
+ cancelpacket.cancelAuthCode = pg_hton32(conn->be_key);
+ if (pqPacketSend(conn, 0, &cancelpacket, packetlen) != STATUS_OK)
+ {
+ libpq_append_conn_error(conn, "could not send cancel packet: %s",
+ SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+ goto error_return;
+ }
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ return PGRES_POLLING_READING;
+ }
+
/*
* Build the startup packet.
*/
@@ -3975,8 +4061,14 @@ keep_going: /* We will come back to here until there is
}
}
- /* We can release the address list now. */
- release_conn_addrinfo(conn);
+ /*
+ * For non cancel requests we can release the address list
+ * now. For cancel requests we never actually resolve
+ * addresses and instead the addrinfo exists for the lifetime
+ * of the connection.
+ */
+ if (!conn->cancelRequest)
+ release_conn_addrinfo(conn);
/*
* Contents of conn->errorMessage are no longer interesting
@@ -4344,6 +4436,7 @@ freePGconn(PGconn *conn)
free(conn->events[i].name);
}
+ release_conn_addrinfo(conn);
pqReleaseConnHosts(conn);
free(conn->client_encoding_initial);
@@ -4495,6 +4588,13 @@ release_conn_addrinfo(PGconn *conn)
static void
sendTerminateConn(PGconn *conn)
{
+ /*
+ * The Postgres cancellation protocol does not have a notion of a
+ * Terminate message, so don't send one.
+ */
+ if (conn->cancelRequest)
+ return;
+
/*
* Note that the protocol doesn't allow us to send Terminate messages
* during the startup phase.
@@ -4548,7 +4648,14 @@ pqClosePGconn(PGconn *conn)
conn->pipelineStatus = PQ_PIPELINE_OFF;
pqClearAsyncResult(conn); /* deallocate result */
pqClearConnErrorState(conn);
- release_conn_addrinfo(conn);
+
+ /*
+ * Release addrinfo, but since cancel requests never change their addrinfo
+ * we don't do that. Otherwise we would have to rebuild it during a
+ * PQcancelReset.
+ */
+ if (!conn->cancelRequest)
+ release_conn_addrinfo(conn);
/* Reset all state obtained from server, too */
pqDropServerData(conn);
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 2c06044a75..09b485bd2b 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -79,7 +79,9 @@ typedef enum
CONNECTION_GSS_STARTUP, /* Negotiating GSSAPI. */
CONNECTION_CHECK_TARGET, /* Internal state: checking target server
* properties. */
- CONNECTION_CHECK_STANDBY /* Checking if server is in standby mode. */
+ CONNECTION_CHECK_STANDBY, /* Checking if server is in standby mode. */
+ CONNECTION_ALLOCATED /* Waiting for connection attempt to be
+ * started. */
} ConnStatusType;
typedef enum
@@ -166,6 +168,11 @@ typedef enum
*/
typedef struct pg_conn PGconn;
+/* PGcancelConn encapsulates a cancel connection to the backend.
+ * The contents of this struct are not supposed to be known to applications.
+ */
+typedef struct pg_cancel_conn PGcancelConn;
+
/* PGresult encapsulates the result of a query (or more precisely, of a single
* SQL command --- a query string given to PQsendQuery can contain multiple
* commands and thus return multiple PGresult objects).
@@ -322,16 +329,34 @@ extern PostgresPollingStatusType PQresetPoll(PGconn *conn);
/* Synchronous (blocking) */
extern void PQreset(PGconn *conn);
+/* Create a PGcancelConn that's used to cancel a query on the given PGconn */
+extern PGcancelConn *PQcancelCreate(PGconn *conn);
+
+/* issue a cancel request in a non-blocking manner */
+extern int PQcancelStart(PGcancelConn *cancelConn);
+
+/* issue a blocking cancel request */
+extern int PQcancelBlocking(PGcancelConn *cancelConn);
+
+/* poll a non-blocking cancel request */
+extern PostgresPollingStatusType PQcancelPoll(PGcancelConn *cancelConn);
+extern ConnStatusType PQcancelStatus(const PGcancelConn *cancelConn);
+extern int PQcancelSocket(const PGcancelConn *cancelConn);
+extern char *PQcancelErrorMessage(const PGcancelConn *cancelConn);
+extern void PQcancelReset(PGcancelConn *cancelConn);
+extern void PQcancelFinish(PGcancelConn *cancelConn);
+
+
/* request a cancel structure */
extern PGcancel *PQgetCancel(PGconn *conn);
/* free a cancel structure */
extern void PQfreeCancel(PGcancel *cancel);
-/* issue a cancel request */
+/* deprecated version of PQcancelBlocking, but one which is signal-safe */
extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
-/* backwards compatible version of PQcancel; not thread-safe */
+/* deprecated version of PQcancel; not thread-safe */
extern int PQrequestCancel(PGconn *conn);
/* Accessor functions for PGconn objects */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 3abcd180d6..9c05f11a6e 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -409,6 +409,10 @@ struct pg_conn
char *require_auth; /* name of the expected auth method */
char *load_balance_hosts; /* load balance over hosts */
+ bool cancelRequest; /* true if this connection is used to send a
+ * cancel request, instead of being a normal
+ * connection that's used for queries */
+
/* Optional file to write trace info to */
FILE *Pfdebug;
int traceFlags;
@@ -669,6 +673,7 @@ extern void pqClosePGconn(PGconn *conn);
extern int pqPacketSend(PGconn *conn, char pack_type,
const void *buf, size_t buf_len);
extern bool pqGetHomeDirectory(char *buf, int bufsize);
+extern bool pqCopyPGconn(PGconn *srcConn, PGconn *dstConn);
extern bool pqParseIntParam(const char *value, int *result, PGconn *conn,
const char *context);
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index c6c7b1c3a1..a17c97bdaf 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -215,6 +215,7 @@ static void
test_cancel(PGconn *conn)
{
PGcancel *cancel;
+ PGcancelConn *cancelConn;
PGconn *monitorConn;
char errorbuf[256];
@@ -251,6 +252,130 @@ test_cancel(PGconn *conn)
pg_fatal("failed to run PQrequestCancel: %s", PQerrorMessage(conn));
confirm_query_canceled(conn);
+ /* test PQcancelBlocking */
+ send_cancellable_query(conn, monitorConn);
+ cancelConn = PQcancelCreate(conn);
+ if (!PQcancelBlocking(cancelConn))
+ pg_fatal("failed to run PQcancelBlocking: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+ PQcancelFinish(cancelConn);
+
+ /* test PQcancelCreate and then polling with PQcancelPoll */
+ send_cancellable_query(conn, monitorConn);
+ cancelConn = PQcancelCreate(conn);
+ if (!PQcancelStart(cancelConn))
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_OK)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+
+ /*
+ * test PQcancelReset works on the cancel connection and it can be reused
+ * afterwards
+ */
+ PQcancelReset(cancelConn);
+
+ send_cancellable_query(conn, monitorConn);
+ if (!PQcancelStart(cancelConn))
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_OK)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+
+ PQcancelFinish(cancelConn);
+
fprintf(stderr, "ok\n");
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d3a7f75b08..504f55a7a1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1762,6 +1762,7 @@ PG_Locale_Strategy
PG_Lock_Status
PG_init_t
PGcancel
+PGcancelConn
PGcmdQueueEntry
PGconn
PGdataValue
--
2.39.2
[text/x-diff] v34-0002-Start-using-new-libpq-cancel-APIs.patch (10.7K, ../../[email protected]/3-v34-0002-Start-using-new-libpq-cancel-APIs.patch)
download | inline diff:
From 2f23b515aa31a0b524db48b68d254bf96dbfe265 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Thu, 14 Dec 2023 13:39:09 +0100
Subject: [PATCH v34 2/2] Start using new libpq cancel APIs
A previous commit introduced new APIs to libpq for cancelling queries.
This replaces the usage of the old APIs in most of the codebase with
these newer ones. This specifically leaves out changes to psql and
pgbench as those would need a much larger refactor to be able to call
them, due to the new functions not being signal-safe.
---
contrib/dblink/dblink.c | 30 +++--
contrib/postgres_fdw/connection.c | 105 +++++++++++++++---
.../postgres_fdw/expected/postgres_fdw.out | 15 +++
contrib/postgres_fdw/sql/postgres_fdw.sql | 7 ++
src/fe_utils/connect_utils.c | 11 +-
src/test/isolation/isolationtester.c | 29 ++---
6 files changed, 145 insertions(+), 52 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 19a362526d..98dcca3e6f 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1346,22 +1346,32 @@ PG_FUNCTION_INFO_V1(dblink_cancel_query);
Datum
dblink_cancel_query(PG_FUNCTION_ARGS)
{
- int res;
PGconn *conn;
- PGcancel *cancel;
- char errbuf[256];
+ PGcancelConn *cancelConn;
+ char *msg;
dblink_init();
conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
- cancel = PQgetCancel(conn);
+ cancelConn = PQcancelCreate(conn);
- res = PQcancel(cancel, errbuf, 256);
- PQfreeCancel(cancel);
+ PG_TRY();
+ {
+ if (!PQcancelBlocking(cancelConn))
+ {
+ msg = pchomp(PQcancelErrorMessage(cancelConn));
+ }
+ else
+ {
+ msg = "OK";
+ }
+ }
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancelConn);
+ }
+ PG_END_TRY();
- if (res == 1)
- PG_RETURN_TEXT_P(cstring_to_text("OK"));
- else
- PG_RETURN_TEXT_P(cstring_to_text(errbuf));
+ PG_RETURN_TEXT_P(cstring_to_text(msg));
}
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 4931ebf591..dcc13dc3b2 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -133,7 +133,7 @@ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue);
static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry);
static void pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel);
static bool pgfdw_cancel_query(PGconn *conn);
-static bool pgfdw_cancel_query_begin(PGconn *conn);
+static bool pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime);
static bool pgfdw_cancel_query_end(PGconn *conn, TimestampTz endtime,
bool consume_input);
static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
@@ -1315,36 +1315,104 @@ pgfdw_cancel_query(PGconn *conn)
endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
CONNECTION_CLEANUP_TIMEOUT);
- if (!pgfdw_cancel_query_begin(conn))
+ if (!pgfdw_cancel_query_begin(conn, endtime))
return false;
return pgfdw_cancel_query_end(conn, endtime, false);
}
static bool
-pgfdw_cancel_query_begin(PGconn *conn)
+pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime)
{
- PGcancel *cancel;
- char errbuf[256];
+ bool timed_out = false;
+ bool failed = false;
+ PGcancelConn *cancel_conn = PQcancelCreate(conn);
- /*
- * Issue cancel request. Unfortunately, there's no good way to limit the
- * amount of time that we might block inside PQgetCancel().
- */
- if ((cancel = PQgetCancel(conn)))
+
+ if (!PQcancelStart(cancel_conn))
{
- if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ PG_TRY();
{
ereport(WARNING,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("could not send cancel request: %s",
- errbuf)));
- PQfreeCancel(cancel);
- return false;
+ pchomp(PQcancelErrorMessage(cancel_conn)))));
}
- PQfreeCancel(cancel);
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancel_conn);
+ }
+ PG_END_TRY();
+ return false;
}
- return true;
+ /* In what follows, do not leak any PGcancelConn on an error. */
+ PG_TRY();
+ {
+ while (true)
+ {
+ TimestampTz now = GetCurrentTimestamp();
+ long cur_timeout;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancel_conn);
+ int waitEvents = WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH;
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ /* If timeout has expired, give up, else get sleep time. */
+ cur_timeout = TimestampDifferenceMilliseconds(now, endtime);
+ if (cur_timeout <= 0)
+ {
+ timed_out = true;
+ failed = true;
+ goto exit;
+ }
+
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ waitEvents |= WL_SOCKET_READABLE;
+ break;
+ case PGRES_POLLING_WRITING:
+ waitEvents |= WL_SOCKET_WRITEABLE;
+ break;
+ default:
+ failed = true;
+ goto exit;
+ }
+
+ /* Sleep until there's something to do */
+ WaitLatchOrSocket(MyLatch, waitEvents, PQcancelSocket(cancel_conn),
+ cur_timeout, PG_WAIT_EXTENSION);
+ ResetLatch(MyLatch);
+
+ CHECK_FOR_INTERRUPTS();
+ }
+exit: ;
+ if (failed)
+ {
+ if (timed_out)
+ {
+ ereport(WARNING,
+ (errmsg("could not cancel request due to timeout")));
+ }
+ else
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not send cancel request: %s",
+ pchomp(PQcancelErrorMessage(cancel_conn)))));
+ }
+ }
+ }
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancel_conn);
+ }
+ PG_END_TRY();
+
+ return !failed;
}
static bool
@@ -1685,7 +1753,10 @@ pgfdw_abort_cleanup_begin(ConnCacheEntry *entry, bool toplevel,
*/
if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
{
- if (!pgfdw_cancel_query_begin(entry->conn))
+ TimestampTz endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
+ CONNECTION_CLEANUP_TIMEOUT);
+
+ if (!pgfdw_cancel_query_begin(entry->conn, endtime))
return false; /* Unable to cancel running query */
*cancel_requested = lappend(*cancel_requested, entry);
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 58a603ac56..e03160bd97 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2739,6 +2739,21 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
(10 rows)
ALTER VIEW v4 OWNER TO regress_view_owner;
+-- Make sure this big CROSS JOIN query is pushed down
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: (count(*))
+ Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5))
+ Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE))
+(4 rows)
+
+-- Make sure query cancellation works
+SET statement_timeout = '10ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+ERROR: canceling statement due to statement timeout
+RESET statement_timeout;
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e3d147de6d..2626e68cc6 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -737,6 +737,13 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10;
ALTER VIEW v4 OWNER TO regress_view_owner;
+-- Make sure this big CROSS JOIN query is pushed down
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+-- Make sure query cancellation works
+SET statement_timeout = '10ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+RESET statement_timeout;
+
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
diff --git a/src/fe_utils/connect_utils.c b/src/fe_utils/connect_utils.c
index 808d54461f..5ed9f3ba17 100644
--- a/src/fe_utils/connect_utils.c
+++ b/src/fe_utils/connect_utils.c
@@ -157,19 +157,14 @@ connectMaintenanceDatabase(ConnParams *cparams,
void
disconnectDatabase(PGconn *conn)
{
- char errbuf[256];
-
Assert(conn != NULL);
if (PQtransactionStatus(conn) == PQTRANS_ACTIVE)
{
- PGcancel *cancel;
+ PGcancelConn *cancelConn = PQcancelCreate(conn);
- if ((cancel = PQgetCancel(conn)))
- {
- (void) PQcancel(cancel, errbuf, sizeof(errbuf));
- PQfreeCancel(cancel);
- }
+ (void) PQcancelBlocking(cancelConn);
+ PQcancelFinish(cancelConn);
}
PQfinish(conn);
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index ed110f740f..0b342b5c2b 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -946,26 +946,21 @@ try_complete_step(TestSpec *testspec, PermutationStep *pstep, int flags)
*/
if (td > max_step_wait && !canceled)
{
- PGcancel *cancel = PQgetCancel(conn);
+ PGcancelConn *cancel_conn = PQcancelCreate(conn);
- if (cancel != NULL)
+ if (PQcancelBlocking(cancel_conn))
{
- char buf[256];
-
- if (PQcancel(cancel, buf, sizeof(buf)))
- {
- /*
- * print to stdout not stderr, as this should appear
- * in the test case's results
- */
- printf("isolationtester: canceling step %s after %d seconds\n",
- step->name, (int) (td / USECS_PER_SEC));
- canceled = true;
- }
- else
- fprintf(stderr, "PQcancel failed: %s\n", buf);
- PQfreeCancel(cancel);
+ /*
+ * print to stdout not stderr, as this should appear in
+ * the test case's results
+ */
+ printf("isolationtester: canceling step %s after %d seconds\n",
+ step->name, (int) (td / USECS_PER_SEC));
+ canceled = true;
}
+ else
+ fprintf(stderr, "PQcancel failed: %s\n", PQcancelErrorMessage(cancel_conn));
+ PQcancelFinish(cancel_conn);
}
/*
--
2.39.2
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2024-03-12 09:53 Jelte Fennema-Nio <[email protected]>
parent: Alvaro Herrera <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Jelte Fennema-Nio @ 2024-03-12 09:53 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Denis Laxalde <[email protected]>; vignesh C <[email protected]>; Thomas Munro <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; Gregory Stark (as CFM) <[email protected]>; Jelte Fennema <[email protected]>; Daniel Gustafsson <[email protected]>; Peter Eisentraut <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Robert Haas <[email protected]>; [email protected] <[email protected]>
On Tue, 12 Mar 2024 at 10:19, Alvaro Herrera <[email protected]> wrote:
> If we do this and we see conn.status is not ALLOCATED, meaning a cancel
> is already ongoing, shouldn't we leave conn.status alone instead of
> changing to CONNECTION_BAD? I mean, we shouldn't be juggling the elbow
> of whoever's doing that, should we? Maybe just add the error message
> and return 0?
I'd rather fail as hard as possible when someone is using the API
wrongly. Not doing so is bound to cause confusion imho. e.g. if the
state is still CONNECTION_OK because the user forgot to call
PQcancelReset then keeping the connection status "as is" might seem as
if the cancel request succeeded even though nothing happened. So if
the user uses the API incorrectly then I'd rather use all the avenues
possible to indicate that there was an error. Especially since in all
other cases if PQcancelStart returns false CONNECTION_BAD is the
status, and this in turn means that PQconnectPoll will return
PGRES_POLLING_FAILED. So I doubt people will always check the actual
return value of the function to check if an error happened. They might
check PQcancelStatus or PQconnectPoll instead, because that integrates
easier with the rest of their code.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2024-03-12 11:41 Jelte Fennema-Nio <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Jelte Fennema-Nio @ 2024-03-12 11:41 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Denis Laxalde <[email protected]>; vignesh C <[email protected]>; Thomas Munro <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; Gregory Stark (as CFM) <[email protected]>; Jelte Fennema <[email protected]>; Daniel Gustafsson <[email protected]>; Peter Eisentraut <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Robert Haas <[email protected]>; [email protected] <[email protected]>
On Tue, 12 Mar 2024 at 10:53, Jelte Fennema-Nio <[email protected]> wrote:
> I'd rather fail as hard as possible when someone is using the API
> wrongly.
To be clear, this is my way of looking at it. If you feel strongly
about that we should not change conn.status, I'm fine with making that
change to the patchset.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2024-03-12 14:04 Jelte Fennema-Nio <[email protected]>
parent: Alvaro Herrera <[email protected]>
1 sibling, 2 replies; 10+ messages in thread
From: Jelte Fennema-Nio @ 2024-03-12 14:04 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Denis Laxalde <[email protected]>; vignesh C <[email protected]>; Thomas Munro <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; Gregory Stark (as CFM) <[email protected]>; Jelte Fennema <[email protected]>; Daniel Gustafsson <[email protected]>; Peter Eisentraut <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Robert Haas <[email protected]>; [email protected] <[email protected]>
On Tue, 12 Mar 2024 at 10:19, Alvaro Herrera <[email protected]> wrote:
> Here's a last one for the cfbot.
Thanks for committing the first 3 patches btw. Attached a tiny change
to 0001, which adds "(backing struct for PGcancelConn)" to the comment
on pg_cancel_conn.
Attachments:
[application/x-patch] v35-0002-Start-using-new-libpq-cancel-APIs.patch (10.7K, ../../CAGECzQRbCq7y7TDBFao21ngnUw5VsBgdFKQ0QJXFCeNwSyf-+Q@mail.gmail.com/2-v35-0002-Start-using-new-libpq-cancel-APIs.patch)
download | inline diff:
From d340fde6883a249fd7c1a90033675a3b5edb603e Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Thu, 14 Dec 2023 13:39:09 +0100
Subject: [PATCH v35 2/2] Start using new libpq cancel APIs
A previous commit introduced new APIs to libpq for cancelling queries.
This replaces the usage of the old APIs in most of the codebase with
these newer ones. This specifically leaves out changes to psql and
pgbench as those would need a much larger refactor to be able to call
them, due to the new functions not being signal-safe.
---
contrib/dblink/dblink.c | 30 +++--
contrib/postgres_fdw/connection.c | 105 +++++++++++++++---
.../postgres_fdw/expected/postgres_fdw.out | 15 +++
contrib/postgres_fdw/sql/postgres_fdw.sql | 7 ++
src/fe_utils/connect_utils.c | 11 +-
src/test/isolation/isolationtester.c | 29 ++---
6 files changed, 145 insertions(+), 52 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 19a362526d2..98dcca3e6fd 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1346,22 +1346,32 @@ PG_FUNCTION_INFO_V1(dblink_cancel_query);
Datum
dblink_cancel_query(PG_FUNCTION_ARGS)
{
- int res;
PGconn *conn;
- PGcancel *cancel;
- char errbuf[256];
+ PGcancelConn *cancelConn;
+ char *msg;
dblink_init();
conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
- cancel = PQgetCancel(conn);
+ cancelConn = PQcancelCreate(conn);
- res = PQcancel(cancel, errbuf, 256);
- PQfreeCancel(cancel);
+ PG_TRY();
+ {
+ if (!PQcancelBlocking(cancelConn))
+ {
+ msg = pchomp(PQcancelErrorMessage(cancelConn));
+ }
+ else
+ {
+ msg = "OK";
+ }
+ }
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancelConn);
+ }
+ PG_END_TRY();
- if (res == 1)
- PG_RETURN_TEXT_P(cstring_to_text("OK"));
- else
- PG_RETURN_TEXT_P(cstring_to_text(errbuf));
+ PG_RETURN_TEXT_P(cstring_to_text(msg));
}
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 4931ebf5915..dcc13dc3b24 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -133,7 +133,7 @@ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue);
static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry);
static void pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel);
static bool pgfdw_cancel_query(PGconn *conn);
-static bool pgfdw_cancel_query_begin(PGconn *conn);
+static bool pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime);
static bool pgfdw_cancel_query_end(PGconn *conn, TimestampTz endtime,
bool consume_input);
static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
@@ -1315,36 +1315,104 @@ pgfdw_cancel_query(PGconn *conn)
endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
CONNECTION_CLEANUP_TIMEOUT);
- if (!pgfdw_cancel_query_begin(conn))
+ if (!pgfdw_cancel_query_begin(conn, endtime))
return false;
return pgfdw_cancel_query_end(conn, endtime, false);
}
static bool
-pgfdw_cancel_query_begin(PGconn *conn)
+pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime)
{
- PGcancel *cancel;
- char errbuf[256];
+ bool timed_out = false;
+ bool failed = false;
+ PGcancelConn *cancel_conn = PQcancelCreate(conn);
- /*
- * Issue cancel request. Unfortunately, there's no good way to limit the
- * amount of time that we might block inside PQgetCancel().
- */
- if ((cancel = PQgetCancel(conn)))
+
+ if (!PQcancelStart(cancel_conn))
{
- if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ PG_TRY();
{
ereport(WARNING,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("could not send cancel request: %s",
- errbuf)));
- PQfreeCancel(cancel);
- return false;
+ pchomp(PQcancelErrorMessage(cancel_conn)))));
}
- PQfreeCancel(cancel);
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancel_conn);
+ }
+ PG_END_TRY();
+ return false;
}
- return true;
+ /* In what follows, do not leak any PGcancelConn on an error. */
+ PG_TRY();
+ {
+ while (true)
+ {
+ TimestampTz now = GetCurrentTimestamp();
+ long cur_timeout;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancel_conn);
+ int waitEvents = WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH;
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ /* If timeout has expired, give up, else get sleep time. */
+ cur_timeout = TimestampDifferenceMilliseconds(now, endtime);
+ if (cur_timeout <= 0)
+ {
+ timed_out = true;
+ failed = true;
+ goto exit;
+ }
+
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ waitEvents |= WL_SOCKET_READABLE;
+ break;
+ case PGRES_POLLING_WRITING:
+ waitEvents |= WL_SOCKET_WRITEABLE;
+ break;
+ default:
+ failed = true;
+ goto exit;
+ }
+
+ /* Sleep until there's something to do */
+ WaitLatchOrSocket(MyLatch, waitEvents, PQcancelSocket(cancel_conn),
+ cur_timeout, PG_WAIT_EXTENSION);
+ ResetLatch(MyLatch);
+
+ CHECK_FOR_INTERRUPTS();
+ }
+exit: ;
+ if (failed)
+ {
+ if (timed_out)
+ {
+ ereport(WARNING,
+ (errmsg("could not cancel request due to timeout")));
+ }
+ else
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not send cancel request: %s",
+ pchomp(PQcancelErrorMessage(cancel_conn)))));
+ }
+ }
+ }
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancel_conn);
+ }
+ PG_END_TRY();
+
+ return !failed;
}
static bool
@@ -1685,7 +1753,10 @@ pgfdw_abort_cleanup_begin(ConnCacheEntry *entry, bool toplevel,
*/
if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
{
- if (!pgfdw_cancel_query_begin(entry->conn))
+ TimestampTz endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
+ CONNECTION_CLEANUP_TIMEOUT);
+
+ if (!pgfdw_cancel_query_begin(entry->conn, endtime))
return false; /* Unable to cancel running query */
*cancel_requested = lappend(*cancel_requested, entry);
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 58a603ac56f..e03160bd975 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2739,6 +2739,21 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
(10 rows)
ALTER VIEW v4 OWNER TO regress_view_owner;
+-- Make sure this big CROSS JOIN query is pushed down
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: (count(*))
+ Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5))
+ Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE))
+(4 rows)
+
+-- Make sure query cancellation works
+SET statement_timeout = '10ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+ERROR: canceling statement due to statement timeout
+RESET statement_timeout;
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e3d147de6da..2626e68cc69 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -737,6 +737,13 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10;
ALTER VIEW v4 OWNER TO regress_view_owner;
+-- Make sure this big CROSS JOIN query is pushed down
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+-- Make sure query cancellation works
+SET statement_timeout = '10ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+RESET statement_timeout;
+
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
diff --git a/src/fe_utils/connect_utils.c b/src/fe_utils/connect_utils.c
index 808d54461fd..5ed9f3ba17b 100644
--- a/src/fe_utils/connect_utils.c
+++ b/src/fe_utils/connect_utils.c
@@ -157,19 +157,14 @@ connectMaintenanceDatabase(ConnParams *cparams,
void
disconnectDatabase(PGconn *conn)
{
- char errbuf[256];
-
Assert(conn != NULL);
if (PQtransactionStatus(conn) == PQTRANS_ACTIVE)
{
- PGcancel *cancel;
+ PGcancelConn *cancelConn = PQcancelCreate(conn);
- if ((cancel = PQgetCancel(conn)))
- {
- (void) PQcancel(cancel, errbuf, sizeof(errbuf));
- PQfreeCancel(cancel);
- }
+ (void) PQcancelBlocking(cancelConn);
+ PQcancelFinish(cancelConn);
}
PQfinish(conn);
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index ed110f740f1..0b342b5c2bb 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -946,26 +946,21 @@ try_complete_step(TestSpec *testspec, PermutationStep *pstep, int flags)
*/
if (td > max_step_wait && !canceled)
{
- PGcancel *cancel = PQgetCancel(conn);
+ PGcancelConn *cancel_conn = PQcancelCreate(conn);
- if (cancel != NULL)
+ if (PQcancelBlocking(cancel_conn))
{
- char buf[256];
-
- if (PQcancel(cancel, buf, sizeof(buf)))
- {
- /*
- * print to stdout not stderr, as this should appear
- * in the test case's results
- */
- printf("isolationtester: canceling step %s after %d seconds\n",
- step->name, (int) (td / USECS_PER_SEC));
- canceled = true;
- }
- else
- fprintf(stderr, "PQcancel failed: %s\n", buf);
- PQfreeCancel(cancel);
+ /*
+ * print to stdout not stderr, as this should appear in
+ * the test case's results
+ */
+ printf("isolationtester: canceling step %s after %d seconds\n",
+ step->name, (int) (td / USECS_PER_SEC));
+ canceled = true;
}
+ else
+ fprintf(stderr, "PQcancel failed: %s\n", PQcancelErrorMessage(cancel_conn));
+ PQcancelFinish(cancel_conn);
}
/*
--
2.34.1
[application/x-patch] v35-0001-libpq-Add-encrypted-and-non-blocking-query-cance.patch (48.5K, ../../CAGECzQRbCq7y7TDBFao21ngnUw5VsBgdFKQ0QJXFCeNwSyf-+Q@mail.gmail.com/3-v35-0001-libpq-Add-encrypted-and-non-blocking-query-cance.patch)
download | inline diff:
From 1c2becaff422b66bc9c263fcdf5c318736f147f6 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email protected]>
Date: Tue, 12 Mar 2024 10:09:25 +0100
Subject: [PATCH v35 1/2] libpq: Add encrypted and non-blocking query
cancellation routines
The existing PQcancel API uses blocking IO, which makes PQcancel
impossible to use in an event loop based codebase without blocking the
event loop until the call returns. It also doesn't encrypt the
connection over which the cancel request is sent, even when the original
connection required encryption.
This commit adds a PQcancelConn struct and assorted functions, which
provide a better mechanism of sending cancel requests; in particular all
the encryption used in the original connection are also used in the
cancel connection. The main entry points are:
- PQcancelCreate creates the PQcancelConn based on the original
connection (but does not establish an actual connection).
- PQcancelStart can be used to initiate non-blocking cancel requests,
using encryption if the original connection did so, which must be
pumped using
- PQcancelPoll.
- PQcancelReset puts a PQcancelConn back in state so that it can be
reused to send a new cancel request to the same connection.
- PQcancelBlocking is a simpler-to-use blocking API that still uses
encryption.
Additional functions are
- PQcancelStatus, mimicks PQstatus;
- PQcancelSocket, mimicks PQcancelSocket;
- PQcancelErrorMessage, mimicks PQerrorMessage;
- PQcancelFinish, mimicks PQfinish.
Author: Jelte Fennema-Nio <[email protected]>
Reviewed-by: Denis Laxalde <[email protected]>
Discussion: https://postgr.es/m/AM5PR83MB0178D3B31CA1B6EC4A8ECC42F7529@AM5PR83MB0178.EURPRD83.prod.outlook.com
---
doc/src/sgml/libpq.sgml | 461 ++++++++++++++++--
src/interfaces/libpq/exports.txt | 9 +
src/interfaces/libpq/fe-cancel.c | 297 ++++++++++-
src/interfaces/libpq/fe-connect.c | 129 ++++-
src/interfaces/libpq/libpq-fe.h | 31 +-
src/interfaces/libpq/libpq-int.h | 5 +
.../modules/libpq_pipeline/libpq_pipeline.c | 125 +++++
src/tools/pgindent/typedefs.list | 1 +
8 files changed, 1013 insertions(+), 45 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index a2bbf33d029..373d0dc3223 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -265,7 +265,7 @@ PGconn *PQsetdb(char *pghost,
<varlistentry id="libpq-PQconnectStartParams">
<term><function>PQconnectStartParams</function><indexterm><primary>PQconnectStartParams</primary></indexterm></term>
<term><function>PQconnectStart</function><indexterm><primary>PQconnectStart</primary></indexterm></term>
- <term><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
+ <term id="libpq-PQconnectPoll"><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
<listitem>
<para>
<indexterm><primary>nonblocking connection</primary></indexterm>
@@ -5287,7 +5287,7 @@ int PQisBusy(PGconn *conn);
<xref linkend="libpq-PQsendQuery"/>/<xref linkend="libpq-PQgetResult"/>
can also attempt to cancel a command that is still being processed
by the server; see <xref linkend="libpq-cancel"/>. But regardless of
- the return value of <xref linkend="libpq-PQcancel"/>, the application
+ the return value of <xref linkend="libpq-PQcancelBlocking"/>, the application
must continue with the normal result-reading sequence using
<xref linkend="libpq-PQgetResult"/>. A successful cancellation will
simply cause the command to terminate sooner than it would have
@@ -6034,10 +6034,402 @@ int PQsetSingleRowMode(PGconn *conn);
<secondary>SQL command</secondary>
</indexterm>
- <para>
- A client application can request cancellation of a command that is
- still being processed by the server, using the functions described in
- this section.
+ <sect2 id="libpq-cancel-conn">
+ <title>Functions for Sending Cancel Requests</title>
+ <variablelist>
+ <varlistentry id="libpq-PQcancelCreate">
+ <term><function>PQcancelCreate</function><indexterm><primary>PQcancelCreate</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Prepares a connection over which a cancel request can be sent.
+<synopsis>
+PGcancelConn *PQcancelCreate(PGconn *conn);
+</synopsis>
+ </para>
+
+ <para>
+ <xref linkend="libpq-PQcancelCreate"/> creates a
+ <structname>PGcancelConn</structname><indexterm><primary>PGcancelConn</primary></indexterm>
+ object, but it won't instantly start sending a cancel request over this
+ connection. A cancel request can be sent over this connection in a
+ blocking manner using <xref linkend="libpq-PQcancelBlocking"/> and in a
+ non-blocking manner using <xref linkend="libpq-PQcancelStart"/>.
+ The return value can be passed to <xref linkend="libpq-PQcancelStatus"/>
+ to check if the <structname>PGcancelConn</structname> object was
+ created successfully. The <structname>PGcancelConn</structname> object
+ is an opaque structure that is not meant to be accessed directly by the
+ application. This <structname>PGcancelConn</structname> object can be
+ used to cancel the query that's running on the original connection in a
+ thread-safe way.
+ </para>
+
+ <para>
+ Many connection parameters of the original client will be reused when
+ setting up the connection for the cancel request. Importantly, if the
+ original connection requires encryption of the connection and/or
+ verification of the target host (using <literal>sslmode</literal> or
+ <literal>gssencmode</literal>), then the connection for the cancel
+ request is made with these same requirements. Any connection options
+ that are only used during authentication or after authentication of the
+ client are ignored though, because cancellation requests do not require
+ authentication and the connection is closed right after the cancellation
+ request is submitted.
+ </para>
+
+ <para>
+ Note that when <function>PQcancelCreate</function> returns a non-null
+ pointer, you must call <xref linkend="libpq-PQcancelFinish"/> when you
+ are finished with it, in order to dispose of the structure and any
+ associated memory blocks. This must be done even if the cancel request
+ failed or was abandoned.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelBlocking">
+ <term><function>PQcancelBlocking</function><indexterm><primary>PQcancelBlocking</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Requests that the server abandons processing of the current command in a blocking manner.
+<synopsis>
+int PQcancelBlocking(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The request is made over the given <structname>PGcancelConn</structname>,
+ which needs to be created with <xref linkend="libpq-PQcancelCreate"/>.
+ The return value of <xref linkend="libpq-PQcancelBlocking"/>
+ is 1 if the cancel request was successfully
+ dispatched and 0 if not. If it was unsuccessful, the error message can be
+ retrieved using <xref linkend="libpq-PQcancelErrorMessage"/>.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelStart">
+ <term><function>PQcancelStart</function><indexterm><primary>PQcancelStart</primary></indexterm></term>
+ <term id="libpq-PQcancelPoll"><function>PQcancelPoll</function><indexterm><primary>PQcancelPoll</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Requests that the server abandons processing of the current command in a non-blocking manner.
+<synopsis>
+int PQcancelStart(PGcancelConn *cancelConn);
+
+PostgresPollingStatusType PQcancelPoll(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The request is made over the given <structname>PGcancelConn</structname>,
+ which needs to be created with <xref linkend="libpq-PQcancelCreate"/>.
+ The return value of <xref linkend="libpq-PQcancelStart"/>
+ is 1 if the cancellation request could be started and 0 if not.
+ If it was unsuccessful, the error message can be
+ retrieved using <xref linkend="libpq-PQcancelErrorMessage"/>.
+ </para>
+
+ <para>
+ If <function>PQcancelStart</function> succeeds, the next stage
+ is to poll <application>libpq</application> so that it can proceed with
+ the cancel connection sequence.
+ Use <xref linkend="libpq-PQcancelSocket"/> to obtain the descriptor of the
+ socket underlying the database connection.
+ (Caution: do not assume that the socket remains the same
+ across <function>PQcancelPoll</function> calls.)
+ Loop thus: If <function>PQcancelPoll(cancelConn)</function> last returned
+ <symbol>PGRES_POLLING_READING</symbol>, wait until the socket is ready to
+ read (as indicated by <function>select()</function>, <function>poll()</function>, or
+ similar system function).
+ Then call <function>PQcancelPoll(cancelConn)</function> again.
+ Conversely, if <function>PQcancelPoll(cancelConn)</function> last returned
+ <symbol>PGRES_POLLING_WRITING</symbol>, wait until the socket is ready
+ to write, then call <function>PQcancelPoll(cancelConn)</function> again.
+ On the first iteration, i.e., if you have yet to call
+ <function>PQcancelPoll(cancelConn)</function>, behave as if it last returned
+ <symbol>PGRES_POLLING_WRITING</symbol>. Continue this loop until
+ <function>PQcancelPoll(cancelConn)</function> returns
+ <symbol>PGRES_POLLING_FAILED</symbol>, indicating the connection procedure
+ has failed, or <symbol>PGRES_POLLING_OK</symbol>, indicating cancel
+ request was successfully dispatched.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ <para>
+ At any time during connection, the status of the connection can be
+ checked by calling <xref linkend="libpq-PQcancelStatus"/>. If this call returns <symbol>CONNECTION_BAD</symbol>, then the
+ cancel procedure has failed; if the call returns <function>CONNECTION_OK</function>, then cancel request was successfully dispatched. Both of these states are equally detectable
+ from the return value of <function>PQcancelPoll</function>, described above. Other states might also occur
+ during (and only during) an asynchronous connection procedure. These
+ indicate the current stage of the connection procedure and might be useful
+ to provide feedback to the user for example. These statuses are:
+
+ <variablelist>
+ <varlistentry id="libpq-connection-allocated">
+ <term><symbol>CONNECTION_ALLOCATED</symbol></term>
+ <listitem>
+ <para>
+ Waiting for a call to <xref linkend="libpq-PQcancelStart"/> or
+ <xref linkend="libpq-PQcancelBlocking"/>, to actually open the
+ socket. This is the connection state right after
+ calling <xref linkend="libpq-PQcancelCreate"/>
+ or <xref linkend="libpq-PQcancelReset"/>. No connection to the
+ server has been initiated yet at this point. To actually start
+ sending the cancel request use <xref linkend="libpq-PQcancelStart"/> or
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-started">
+ <term><symbol>CONNECTION_STARTED</symbol></term>
+ <listitem>
+ <para>
+ Waiting for connection to be made.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-made">
+ <term><symbol>CONNECTION_MADE</symbol></term>
+ <listitem>
+ <para>
+ Connection OK; waiting to send.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-awaiting-response">
+ <term><symbol>CONNECTION_AWAITING_RESPONSE</symbol></term>
+ <listitem>
+ <para>
+ Waiting for a response from the server.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-ssl-startup">
+ <term><symbol>CONNECTION_SSL_STARTUP</symbol></term>
+ <listitem>
+ <para>
+ Negotiating SSL encryption.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-gss-startup">
+ <term><symbol>CONNECTION_GSS_STARTUP</symbol></term>
+ <listitem>
+ <para>
+ Negotiating GSS encryption.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ Note that, although these constants will remain (in order to maintain
+ compatibility), an application should never rely upon these occurring in a
+ particular order, or at all, or on the status always being one of these
+ documented values. An application might do something like this:
+<programlisting>
+switch(PQcancelStatus(conn))
+{
+ case CONNECTION_STARTED:
+ feedback = "Connecting...";
+ break;
+
+ case CONNECTION_MADE:
+ feedback = "Connected to server...";
+ break;
+.
+.
+.
+ default:
+ feedback = "Connecting...";
+}
+</programlisting>
+ </para>
+
+ <para>
+ The <literal>connect_timeout</literal> connection parameter is ignored
+ when using <function>PQcancelPoll</function>; it is the application's
+ responsibility to decide whether an excessive amount of time has elapsed.
+ Otherwise, <function>PQcancelStart</function> followed by a
+ <function>PQcancelPoll</function> loop is equivalent to
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelStatus">
+ <term><function>PQcancelStatus</function><indexterm><primary>PQcancelStatus</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Returns the status of the cancel connection.
+<synopsis>
+ConnStatusType PQcancelStatus(const PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The status can be one of a number of values. However, only three of
+ these are seen outside of an asynchronous cancel procedure:
+ <literal>CONNECTION_ALLOCATED</literal>,
+ <literal>CONNECTION_OK</literal> and
+ <literal>CONNECTION_BAD</literal>. The initial state of a
+ <function>PGcancelConn</function> that's successfully created using
+ <xref linkend="libpq-PQcancelCreate"/> is <literal>CONNECTION_ALLOCATED</literal>.
+ A cancel request that was successfully dispatched
+ has the status <literal>CONNECTION_OK</literal>. A failed
+ cancel attempt is signaled by status
+ <literal>CONNECTION_BAD</literal>. An OK status will
+ remain so until <xref linkend="libpq-PQcancelFinish"/> or
+ <xref linkend="libpq-PQcancelReset"/> is called.
+ </para>
+
+ <para>
+ See the entry for <xref linkend="libpq-PQcancelStart"/> and <xref
+ linkend="libpq-PQcancelPoll"/> with regards to other status codes that
+ might be returned.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelSocket">
+ <term><function>PQcancelSocket</function><indexterm><primary>PQcancelSocket</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Obtains the file descriptor number of the cancel connection socket to
+ the server. A valid descriptor will be greater than or equal
+ to 0; a result of -1 indicates that no server connection is
+ currently open. This might change as a result of calling all of the
+ functions in this section on the (except for
+ <xref linkend="libpq-PQcancelErrorMessage"/> and
+ <function>PQcancelSocket</function> itself).
+<synopsis>
+int PQcancelSocket(const PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelErrorMessage">
+ <term><function>PQcancelErrorMessage</function><indexterm><primary>PQcancelErrorMessage</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ <indexterm><primary>error message</primary></indexterm> Returns the error message
+ most recently generated by an operation on the cancel connection.
+<synopsis>
+char *PQcancelErrorMessage(const PGcancelConn *cancelconn);
+</synopsis>
+ </para>
+
+ <para>
+ Nearly all <application>libpq</application> functions that take a
+ <structname>PGcancelConn</structname> will set a message for
+ <xref linkend="libpq-PQcancelErrorMessage"/> if they fail. Note that by
+ <application>libpq</application> convention, a nonempty
+ <xref linkend="libpq-PQcancelErrorMessage"/> result can consist of multiple lines,
+ and will include a trailing newline. The caller should not free
+ the result directly. It will be freed when the associated
+ <structname>PGcancelConn</structname> handle is passed to
+ <xref linkend="libpq-PQcancelFinish"/>. The result string should not be
+ expected to remain the same across operations on the
+ <literal>PGcancelConn</literal> structure.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelFinish">
+ <term><function>PQcancelFinish</function><indexterm><primary>PQcancelFinish</primary></indexterm></term>
+ <listitem>
+ <para>
+ Closes the cancel connection (if it did not finish sending the cancel
+ request yet). Also frees memory used by the <structname>PGcancelConn</structname>
+ object.
+<synopsis>
+void PQcancelFinish(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ Note that even if the cancel attempt fails (as
+ indicated by <xref linkend="libpq-PQcancelStatus"/>), the application should call <xref linkend="libpq-PQcancelFinish"/>
+ to free the memory used by the <structname>PGcancelConn</structname> object.
+ The <structname>PGcancelConn</structname> pointer must not be used again after
+ <xref linkend="libpq-PQcancelFinish"/> has been called.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelReset">
+ <term><function>PQcancelReset</function><indexterm><primary>PQcancelReset</primary></indexterm></term>
+ <listitem>
+ <para>
+ Resets the <symbol>PGcancelConn</symbol> so it can be reused for a new
+ cancel connection.
+<synopsis>
+void PQcancelReset(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ If the <symbol>PGcancelConn</symbol> is currently used to send a cancel
+ request, then this connection is closed. It will then prepare the
+ <symbol>PGcancelConn</symbol> object such that it can be used to send a
+ new cancel request. This can be used to create one <symbol>PGcancelConn</symbol>
+ for a <symbol>PGconn</symbol> and reuse that multiple times throughout
+ the lifetime of the original <symbol>PGconn</symbol>.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </sect2>
+
+ <sect2 id="libpq-cancel-deprecated">
+ <title>Obsolete Functions for Sending Cancel Requests</title>
+
+ <para>
+ These functions represent older methods of sending cancel requests.
+ Although they still work, they are deprecated due to not sending the cancel
+ requests in an encrypted manner, even when the original connection
+ specified <literal>sslmode</literal> or <literal>gssencmode</literal> to
+ require encryption. Thus these older methods are heavily discouraged from
+ being used in new code, and it is recommended to change existing code to
+ use the new functions instead.
+ </para>
<variablelist>
<varlistentry id="libpq-PQgetCancel">
@@ -6046,7 +6438,7 @@ int PQsetSingleRowMode(PGconn *conn);
<listitem>
<para>
Creates a data structure containing the information needed to cancel
- a command issued through a particular database connection.
+ a command using <xref linkend="libpq-PQcancel"/>.
<synopsis>
PGcancel *PQgetCancel(PGconn *conn);
</synopsis>
@@ -6088,36 +6480,37 @@ void PQfreeCancel(PGcancel *cancel);
<listitem>
<para>
- Requests that the server abandon processing of the current command.
-<synopsis>
+ <xref linkend="libpq-PQrequestCancel"/> is a deprecated and insecure
+ variant of <xref linkend="libpq-PQcancelBlocking"/>, but one that can be
+ used safely from within a signal handler. <synopsis>
int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
</synopsis>
</para>
<para>
- The return value is 1 if the cancel request was successfully
- dispatched and 0 if not. If not, <parameter>errbuf</parameter> is filled
- with an explanatory error message. <parameter>errbuf</parameter>
- must be a char array of size <parameter>errbufsize</parameter> (the
- recommended size is 256 bytes).
+ <xref linkend="libpq-PQcancel"/> only exists because of backwards
+ compatibility reasons. <xref linkend="libpq-PQcancelBlocking"/> should be
+ used instead. The only benefit that <xref linkend="libpq-PQcancel"/> has
+ is that it can be safely invoked from a signal handler, if the
+ <parameter>errbuf</parameter> is a local variable in the signal handler.
+ However, this is generally not considered a big enough benefit to be
+ worth the security issues that this function has.
</para>
<para>
- Successful dispatch is no guarantee that the request will have
- any effect, however. If the cancellation is effective, the current
- command will terminate early and return an error result. If the
- cancellation fails (say, because the server was already done
- processing the command), then there will be no visible result at
- all.
+ The <structname>PGcancel</structname> object is read-only as far as
+ <xref linkend="libpq-PQcancel"/> is concerned, so it can also be invoked
+ from a thread that is separate from the one manipulating the
+ <structname>PGconn</structname> object.
</para>
<para>
- <xref linkend="libpq-PQcancel"/> can safely be invoked from a signal
- handler, if the <parameter>errbuf</parameter> is a local variable in the
- signal handler. The <structname>PGcancel</structname> object is read-only
- as far as <xref linkend="libpq-PQcancel"/> is concerned, so it can
- also be invoked from a thread that is separate from the one
- manipulating the <structname>PGconn</structname> object.
+ The return value of <xref linkend="libpq-PQcancel"/>
+ is 1 if the cancel request was successfully
+ dispatched and 0 if not. If not, <parameter>errbuf</parameter> is filled
+ with an explanatory error message. <parameter>errbuf</parameter>
+ must be a char array of size <parameter>errbufsize</parameter> (the
+ recommended size is 256 bytes).
</para>
</listitem>
</varlistentry>
@@ -6129,13 +6522,21 @@ int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
<listitem>
<para>
- <xref linkend="libpq-PQrequestCancel"/> is a deprecated variant of
- <xref linkend="libpq-PQcancel"/>.
+ <xref linkend="libpq-PQrequestCancel"/> is a deprecated and insecure
+ variant of <xref linkend="libpq-PQcancelBlocking"/>.
<synopsis>
int PQrequestCancel(PGconn *conn);
</synopsis>
</para>
+ <para>
+ <xref linkend="libpq-PQrequestCancel"/> only exists because of backwards
+ compatibility reasons. <xref linkend="libpq-PQcancelBlocking"/> should be
+ used instead. There is no benefit to using
+ <xref linkend="libpq-PQrequestCancel"/> over
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+
<para>
Requests that the server abandon processing of the current
command. It operates directly on the
@@ -6150,7 +6551,7 @@ int PQrequestCancel(PGconn *conn);
</listitem>
</varlistentry>
</variablelist>
- </para>
+ </sect2>
</sect1>
@@ -9362,7 +9763,7 @@ int PQisthreadsafe();
The deprecated functions <xref linkend="libpq-PQrequestCancel"/> and
<xref linkend="libpq-PQoidStatus"/> are not thread-safe and should not be
used in multithread programs. <xref linkend="libpq-PQrequestCancel"/>
- can be replaced by <xref linkend="libpq-PQcancel"/>.
+ can be replaced by <xref linkend="libpq-PQcancelBlocking"/>.
<xref linkend="libpq-PQoidStatus"/> can be replaced by
<xref linkend="libpq-PQoidValue"/>.
</para>
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 088592deb16..9fbd3d34074 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -193,3 +193,12 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQchangePassword 192
PQsendPipelineSync 193
+PQcancelBlocking 194
+PQcancelStart 195
+PQcancelCreate 196
+PQcancelPoll 197
+PQcancelStatus 198
+PQcancelSocket 199
+PQcancelErrorMessage 200
+PQcancelReset 201
+PQcancelFinish 202
diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c
index d69b8f9f9f4..f9ea32470ae 100644
--- a/src/interfaces/libpq/fe-cancel.c
+++ b/src/interfaces/libpq/fe-cancel.c
@@ -22,6 +22,18 @@
#include "port/pg_bswap.h"
+/*
+ * pg_cancel_conn (backing struct for PGcancelConn) is a wrapper around a
+ * PGconn to send cancellations using PQcancelBlocking and PQcancelStart. This
+ * isn't just a typedef because we want the compiler to complain when a PGconn
+ * is passed to a function that expects a PGcancelConn, and vice versa.
+ */
+struct pg_cancel_conn
+{
+ PGconn conn;
+};
+
+
/*
* pg_cancel (backing struct for PGcancel) stores all data necessary to send a
* cancel request.
@@ -41,6 +53,289 @@ struct pg_cancel
};
+/*
+ * PQcancelCreate
+ *
+ * Create and return a PGcancelConn, which can be used to securely cancel a
+ * query on the given connection.
+ *
+ * This requires either following the non-blocking flow through
+ * PQcancelStart() and PQcancelPoll(), or the blocking PQcancelBlocking().
+ */
+PGcancelConn *
+PQcancelCreate(PGconn *conn)
+{
+ PGconn *cancelConn = pqMakeEmptyPGconn();
+ pg_conn_host originalHost;
+
+ if (cancelConn == NULL)
+ return NULL;
+
+ /* Check we have an open connection */
+ if (!conn)
+ {
+ libpq_append_conn_error(cancelConn, "passed connection was NULL");
+ return (PGcancelConn *) cancelConn;
+ }
+
+ if (conn->sock == PGINVALID_SOCKET)
+ {
+ libpq_append_conn_error(cancelConn, "passed connection is not open");
+ return (PGcancelConn *) cancelConn;
+ }
+
+ /*
+ * Indicate that this connection is used to send a cancellation
+ */
+ cancelConn->cancelRequest = true;
+
+ if (!pqCopyPGconn(conn, cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Compute derived options
+ */
+ if (!pqConnectOptions2(cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Copy cancellation token data from the original connnection
+ */
+ cancelConn->be_pid = conn->be_pid;
+ cancelConn->be_key = conn->be_key;
+
+ /*
+ * Cancel requests should not iterate over all possible hosts. The request
+ * needs to be sent to the exact host and address that the original
+ * connection used. So we manually create the host and address arrays with
+ * a single element after freeing the host array that we generated from
+ * the connection options.
+ */
+ pqReleaseConnHosts(cancelConn);
+ cancelConn->nconnhost = 1;
+ cancelConn->naddr = 1;
+
+ cancelConn->connhost = calloc(cancelConn->nconnhost, sizeof(pg_conn_host));
+ if (!cancelConn->connhost)
+ goto oom_error;
+
+ originalHost = conn->connhost[conn->whichhost];
+ if (originalHost.host)
+ {
+ cancelConn->connhost[0].host = strdup(originalHost.host);
+ if (!cancelConn->connhost[0].host)
+ goto oom_error;
+ }
+ if (originalHost.hostaddr)
+ {
+ cancelConn->connhost[0].hostaddr = strdup(originalHost.hostaddr);
+ if (!cancelConn->connhost[0].hostaddr)
+ goto oom_error;
+ }
+ if (originalHost.port)
+ {
+ cancelConn->connhost[0].port = strdup(originalHost.port);
+ if (!cancelConn->connhost[0].port)
+ goto oom_error;
+ }
+ if (originalHost.password)
+ {
+ cancelConn->connhost[0].password = strdup(originalHost.password);
+ if (!cancelConn->connhost[0].password)
+ goto oom_error;
+ }
+
+ cancelConn->addr = calloc(cancelConn->naddr, sizeof(AddrInfo));
+ if (!cancelConn->connhost)
+ goto oom_error;
+
+ cancelConn->addr[0].addr = conn->raddr;
+ cancelConn->addr[0].family = conn->raddr.addr.ss_family;
+
+ cancelConn->status = CONNECTION_ALLOCATED;
+ return (PGcancelConn *) cancelConn;
+
+oom_error:
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(cancelConn, "out of memory");
+ return (PGcancelConn *) cancelConn;
+}
+
+
+/*
+ * PQcancelBlocking
+ *
+ * Send a cancellation request in a blocking fashion.
+ * Returns 1 if successful 0 if not.
+ */
+int
+PQcancelBlocking(PGcancelConn *cancelConn)
+{
+ if (!PQcancelStart(cancelConn))
+ return 0;
+ return pqConnectDBComplete(&cancelConn->conn);
+}
+
+/*
+ * PQcancelStart
+ *
+ * Starts sending a cancellation request in a non-blocking fashion. Returns
+ * 1 if successful 0 if not.
+ */
+int
+PQcancelStart(PGcancelConn *cancelConn)
+{
+ if (!cancelConn || cancelConn->conn.status == CONNECTION_BAD)
+ return 0;
+
+ if (cancelConn->conn.status != CONNECTION_ALLOCATED)
+ {
+ libpq_append_conn_error(&cancelConn->conn,
+ "cancel request is already being sent on this connection");
+ cancelConn->conn.status = CONNECTION_BAD;
+ return 0;
+ }
+
+ return pqConnectDBStart(&cancelConn->conn);
+}
+
+/*
+ * PQcancelPoll
+ *
+ * Poll a cancel connection. For usage details see PQconnectPoll.
+ */
+PostgresPollingStatusType
+PQcancelPoll(PGcancelConn *cancelConn)
+{
+ PGconn *conn = &cancelConn->conn;
+ int n;
+
+ /*
+ * We leave most of the connection establishement to PQconnectPoll, since
+ * it's very similar to normal connection establishment. But once we get
+ * to the CONNECTION_AWAITING_RESPONSE we need to start doing our own
+ * thing.
+ */
+ if (conn->status != CONNECTION_AWAITING_RESPONSE)
+ {
+ return PQconnectPoll(conn);
+ }
+
+ /*
+ * At this point we are waiting on the server to close the connection,
+ * which is its way of communicating that the cancel has been handled.
+ */
+
+ n = pqReadData(conn);
+
+ if (n == 0)
+ return PGRES_POLLING_READING;
+
+#ifndef WIN32
+
+ /*
+ * If we receive an error report it, but only if errno is non-zero.
+ * Otherwise we assume it's an EOF, which is what we expect from the
+ * server.
+ *
+ * We skip this for Windows, because Windows is a bit special in its EOF
+ * behaviour for TCP. Sometimes it will error with an ECONNRESET when
+ * there is a clean connection closure. See these threads for details:
+ * https://www.postgresql.org/message-id/flat/90b34057-4176-7bb0-0dbb-9822a5f6425b%40greiz-reinsdorf.de
+ *
+ * https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BOeoETZQ%3DQw5Ub5h3tmwQhBmDA%3DnuNO3KG%3DzWfUypFAw%40mail.gmail.com
+ *
+ * PQcancel ignores such errors and reports success for the cancellation
+ * anyway, so even if this is not always correct we do the same here.
+ */
+ if (n < 0 && errno != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ return PGRES_POLLING_FAILED;
+ }
+#endif
+
+ /*
+ * We don't expect any data, only connection closure. So if we strangely
+ * do receive some data we consider that an error.
+ */
+ if (n > 0)
+ {
+ libpq_append_conn_error(conn, "received unexpected response from server");
+ conn->status = CONNECTION_BAD;
+ return PGRES_POLLING_FAILED;
+ }
+
+ /*
+ * Getting here means that we received an EOF, which is what we were
+ * expecting -- the cancel request has completed.
+ */
+ cancelConn->conn.status = CONNECTION_OK;
+ resetPQExpBuffer(&conn->errorMessage);
+ return PGRES_POLLING_OK;
+}
+
+/*
+ * PQcancelStatus
+ *
+ * Get the status of a cancel connection.
+ */
+ConnStatusType
+PQcancelStatus(const PGcancelConn *cancelConn)
+{
+ return PQstatus(&cancelConn->conn);
+}
+
+/*
+ * PQcancelSocket
+ *
+ * Get the socket of the cancel connection.
+ */
+int
+PQcancelSocket(const PGcancelConn *cancelConn)
+{
+ return PQsocket(&cancelConn->conn);
+}
+
+/*
+ * PQcancelErrorMessage
+ *
+ * Get the socket of the cancel connection.
+ */
+char *
+PQcancelErrorMessage(const PGcancelConn *cancelConn)
+{
+ return PQerrorMessage(&cancelConn->conn);
+}
+
+/*
+ * PQcancelReset
+ *
+ * Resets the cancel connection, so it can be reused to send a new cancel
+ * request.
+ */
+void
+PQcancelReset(PGcancelConn *cancelConn)
+{
+ pqClosePGconn(&cancelConn->conn);
+ cancelConn->conn.status = CONNECTION_ALLOCATED;
+ cancelConn->conn.whichhost = 0;
+ cancelConn->conn.whichaddr = 0;
+ cancelConn->conn.try_next_host = false;
+ cancelConn->conn.try_next_addr = false;
+}
+
+/*
+ * PQcancelFinish
+ *
+ * Closes and frees the cancel connection.
+ */
+void
+PQcancelFinish(PGcancelConn *cancelConn)
+{
+ PQfinish(&cancelConn->conn);
+}
+
/*
* PQgetCancel: get a PGcancel structure corresponding to a connection.
*
@@ -145,7 +440,7 @@ optional_setsockopt(int fd, int protoid, int optid, int value)
/*
- * PQcancel: request query cancel
+ * PQcancel: old, non-encrypted, but signal-safe way of requesting query cancel
*
* The return value is true if the cancel request was successfully
* dispatched, false if not (in which case an error message is available).
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d4e10a0c4f3..8e8634e5baf 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -616,8 +616,17 @@ pqDropServerData(PGconn *conn)
conn->write_failed = false;
free(conn->write_err_msg);
conn->write_err_msg = NULL;
- conn->be_pid = 0;
- conn->be_key = 0;
+
+ /*
+ * Cancel connections need to retain their be_pid and be_key across
+ * PQcancelReset invocations, otherwise they would not have access to the
+ * secret token of the connection they are supposed to cancel.
+ */
+ if (!conn->cancelRequest)
+ {
+ conn->be_pid = 0;
+ conn->be_key = 0;
+ }
}
@@ -923,6 +932,45 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
return true;
}
+/*
+ * Copy over option values from srcConn to dstConn
+ *
+ * Don't put anything cute here --- intelligence should be in
+ * connectOptions2 ...
+ *
+ * Returns true on success. On failure, returns false and sets error message of
+ * dstConn.
+ */
+bool
+pqCopyPGconn(PGconn *srcConn, PGconn *dstConn)
+{
+ const internalPQconninfoOption *option;
+
+ /* copy over connection options */
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ if (option->connofs >= 0)
+ {
+ const char **tmp = (const char **) ((char *) srcConn + option->connofs);
+
+ if (*tmp)
+ {
+ char **dstConnmember = (char **) ((char *) dstConn + option->connofs);
+
+ if (*dstConnmember)
+ free(*dstConnmember);
+ *dstConnmember = strdup(*tmp);
+ if (*dstConnmember == NULL)
+ {
+ libpq_append_conn_error(dstConn, "out of memory");
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+}
+
/*
* connectOptions1
*
@@ -2308,10 +2356,18 @@ pqConnectDBStart(PGconn *conn)
* Set up to try to connect to the first host. (Setting whichhost = -1 is
* a bit of a cheat, but PQconnectPoll will advance it to 0 before
* anything else looks at it.)
+ *
+ * Cancel requests are special though, they should only try one host and
+ * address, and these fields have already been set up in PQcancelCreate, so
+ * leave these fields alone for cancel requests.
*/
- conn->whichhost = -1;
- conn->try_next_addr = false;
- conn->try_next_host = true;
+ if (!conn->cancelRequest)
+ {
+ conn->whichhost = -1;
+ conn->try_next_host = true;
+ conn->try_next_addr = false;
+ }
+
conn->status = CONNECTION_NEEDED;
/* Also reset the target_server_type state if needed */
@@ -2453,7 +2509,10 @@ pqConnectDBComplete(PGconn *conn)
/*
* Now try to advance the state machine.
*/
- flag = PQconnectPoll(conn);
+ if (conn->cancelRequest)
+ flag = PQcancelPoll((PGcancelConn *) conn);
+ else
+ flag = PQconnectPoll(conn);
}
}
@@ -2578,13 +2637,17 @@ keep_going: /* We will come back to here until there is
* Oops, no more hosts.
*
* If we are trying to connect in "prefer-standby" mode, then drop
- * the standby requirement and start over.
+ * the standby requirement and start over. Don't do this for
+ * cancel requests though, since we are certain the list of
+ * servers won't change as the target_server_type option is not
+ * applicable to those connections.
*
* Otherwise, an appropriate error message is already set up, so
* we just need to set the right status.
*/
if (conn->target_server_type == SERVER_TYPE_PREFER_STANDBY &&
- conn->nconnhost > 0)
+ conn->nconnhost > 0 &&
+ !conn->cancelRequest)
{
conn->target_server_type = SERVER_TYPE_PREFER_STANDBY_PASS2;
conn->whichhost = 0;
@@ -3226,6 +3289,29 @@ keep_going: /* We will come back to here until there is
}
#endif /* USE_SSL */
+ /*
+ * For cancel requests this is as far as we need to go in the
+ * connection establishment. Now we can actually send our
+ * cancellation request.
+ */
+ if (conn->cancelRequest)
+ {
+ CancelRequestPacket cancelpacket;
+
+ packetlen = sizeof(cancelpacket);
+ cancelpacket.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
+ cancelpacket.backendPID = pg_hton32(conn->be_pid);
+ cancelpacket.cancelAuthCode = pg_hton32(conn->be_key);
+ if (pqPacketSend(conn, 0, &cancelpacket, packetlen) != STATUS_OK)
+ {
+ libpq_append_conn_error(conn, "could not send cancel packet: %s",
+ SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+ goto error_return;
+ }
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ return PGRES_POLLING_READING;
+ }
+
/*
* Build the startup packet.
*/
@@ -3975,8 +4061,14 @@ keep_going: /* We will come back to here until there is
}
}
- /* We can release the address list now. */
- release_conn_addrinfo(conn);
+ /*
+ * For non cancel requests we can release the address list
+ * now. For cancel requests we never actually resolve
+ * addresses and instead the addrinfo exists for the lifetime
+ * of the connection.
+ */
+ if (!conn->cancelRequest)
+ release_conn_addrinfo(conn);
/*
* Contents of conn->errorMessage are no longer interesting
@@ -4344,6 +4436,7 @@ freePGconn(PGconn *conn)
free(conn->events[i].name);
}
+ release_conn_addrinfo(conn);
pqReleaseConnHosts(conn);
free(conn->client_encoding_initial);
@@ -4495,6 +4588,13 @@ release_conn_addrinfo(PGconn *conn)
static void
sendTerminateConn(PGconn *conn)
{
+ /*
+ * The Postgres cancellation protocol does not have a notion of a
+ * Terminate message, so don't send one.
+ */
+ if (conn->cancelRequest)
+ return;
+
/*
* Note that the protocol doesn't allow us to send Terminate messages
* during the startup phase.
@@ -4548,7 +4648,14 @@ pqClosePGconn(PGconn *conn)
conn->pipelineStatus = PQ_PIPELINE_OFF;
pqClearAsyncResult(conn); /* deallocate result */
pqClearConnErrorState(conn);
- release_conn_addrinfo(conn);
+
+ /*
+ * Release addrinfo, but since cancel requests never change their addrinfo
+ * we don't do that. Otherwise we would have to rebuild it during a
+ * PQcancelReset.
+ */
+ if (!conn->cancelRequest)
+ release_conn_addrinfo(conn);
/* Reset all state obtained from server, too */
pqDropServerData(conn);
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 2c06044a75e..09b485bd2bc 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -79,7 +79,9 @@ typedef enum
CONNECTION_GSS_STARTUP, /* Negotiating GSSAPI. */
CONNECTION_CHECK_TARGET, /* Internal state: checking target server
* properties. */
- CONNECTION_CHECK_STANDBY /* Checking if server is in standby mode. */
+ CONNECTION_CHECK_STANDBY, /* Checking if server is in standby mode. */
+ CONNECTION_ALLOCATED /* Waiting for connection attempt to be
+ * started. */
} ConnStatusType;
typedef enum
@@ -166,6 +168,11 @@ typedef enum
*/
typedef struct pg_conn PGconn;
+/* PGcancelConn encapsulates a cancel connection to the backend.
+ * The contents of this struct are not supposed to be known to applications.
+ */
+typedef struct pg_cancel_conn PGcancelConn;
+
/* PGresult encapsulates the result of a query (or more precisely, of a single
* SQL command --- a query string given to PQsendQuery can contain multiple
* commands and thus return multiple PGresult objects).
@@ -322,16 +329,34 @@ extern PostgresPollingStatusType PQresetPoll(PGconn *conn);
/* Synchronous (blocking) */
extern void PQreset(PGconn *conn);
+/* Create a PGcancelConn that's used to cancel a query on the given PGconn */
+extern PGcancelConn *PQcancelCreate(PGconn *conn);
+
+/* issue a cancel request in a non-blocking manner */
+extern int PQcancelStart(PGcancelConn *cancelConn);
+
+/* issue a blocking cancel request */
+extern int PQcancelBlocking(PGcancelConn *cancelConn);
+
+/* poll a non-blocking cancel request */
+extern PostgresPollingStatusType PQcancelPoll(PGcancelConn *cancelConn);
+extern ConnStatusType PQcancelStatus(const PGcancelConn *cancelConn);
+extern int PQcancelSocket(const PGcancelConn *cancelConn);
+extern char *PQcancelErrorMessage(const PGcancelConn *cancelConn);
+extern void PQcancelReset(PGcancelConn *cancelConn);
+extern void PQcancelFinish(PGcancelConn *cancelConn);
+
+
/* request a cancel structure */
extern PGcancel *PQgetCancel(PGconn *conn);
/* free a cancel structure */
extern void PQfreeCancel(PGcancel *cancel);
-/* issue a cancel request */
+/* deprecated version of PQcancelBlocking, but one which is signal-safe */
extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
-/* backwards compatible version of PQcancel; not thread-safe */
+/* deprecated version of PQcancel; not thread-safe */
extern int PQrequestCancel(PGconn *conn);
/* Accessor functions for PGconn objects */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 3abcd180d6d..9c05f11a6e9 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -409,6 +409,10 @@ struct pg_conn
char *require_auth; /* name of the expected auth method */
char *load_balance_hosts; /* load balance over hosts */
+ bool cancelRequest; /* true if this connection is used to send a
+ * cancel request, instead of being a normal
+ * connection that's used for queries */
+
/* Optional file to write trace info to */
FILE *Pfdebug;
int traceFlags;
@@ -669,6 +673,7 @@ extern void pqClosePGconn(PGconn *conn);
extern int pqPacketSend(PGconn *conn, char pack_type,
const void *buf, size_t buf_len);
extern bool pqGetHomeDirectory(char *buf, int bufsize);
+extern bool pqCopyPGconn(PGconn *srcConn, PGconn *dstConn);
extern bool pqParseIntParam(const char *value, int *result, PGconn *conn,
const char *context);
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index c6c7b1c3a17..a17c97bdaf4 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -215,6 +215,7 @@ static void
test_cancel(PGconn *conn)
{
PGcancel *cancel;
+ PGcancelConn *cancelConn;
PGconn *monitorConn;
char errorbuf[256];
@@ -251,6 +252,130 @@ test_cancel(PGconn *conn)
pg_fatal("failed to run PQrequestCancel: %s", PQerrorMessage(conn));
confirm_query_canceled(conn);
+ /* test PQcancelBlocking */
+ send_cancellable_query(conn, monitorConn);
+ cancelConn = PQcancelCreate(conn);
+ if (!PQcancelBlocking(cancelConn))
+ pg_fatal("failed to run PQcancelBlocking: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+ PQcancelFinish(cancelConn);
+
+ /* test PQcancelCreate and then polling with PQcancelPoll */
+ send_cancellable_query(conn, monitorConn);
+ cancelConn = PQcancelCreate(conn);
+ if (!PQcancelStart(cancelConn))
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_OK)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+
+ /*
+ * test PQcancelReset works on the cancel connection and it can be reused
+ * afterwards
+ */
+ PQcancelReset(cancelConn);
+
+ send_cancellable_query(conn, monitorConn);
+ if (!PQcancelStart(cancelConn))
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_OK)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+
+ PQcancelFinish(cancelConn);
+
fprintf(stderr, "ok\n");
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index a3052a181d1..aa7a25b8f8c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1763,6 +1763,7 @@ PG_Locale_Strategy
PG_Lock_Status
PG_init_t
PGcancel
+PGcancelConn
PGcmdQueueEntry
PGconn
PGdataValue
base-commit: 4945e4ed4a72c3ff41560ccef722c3d70ae07dbb
--
2.34.1
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2024-03-12 15:45 Jelte Fennema-Nio <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Jelte Fennema-Nio @ 2024-03-12 15:45 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Denis Laxalde <[email protected]>; vignesh C <[email protected]>; Thomas Munro <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; Gregory Stark (as CFM) <[email protected]>; Jelte Fennema <[email protected]>; Daniel Gustafsson <[email protected]>; Peter Eisentraut <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Robert Haas <[email protected]>; [email protected] <[email protected]>
On Tue, 12 Mar 2024 at 15:04, Jelte Fennema-Nio <[email protected]> wrote:
> Attached a tiny change to 0001
One more tiny comment change, stating that pg_cancel is used by the
deprecated PQcancel function.
Attachments:
[application/octet-stream] v36-0001-libpq-Add-encrypted-and-non-blocking-query-cance.patch (48.6K, ../../CAGECzQTGqEWO2ER1dh0YHMtyXrmOotQ7K3j5_gE_6VW49iBs0w@mail.gmail.com/2-v36-0001-libpq-Add-encrypted-and-non-blocking-query-cance.patch)
download | inline diff:
From 612e621defb0887f8bc695822b78dbc35f005832 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email protected]>
Date: Tue, 12 Mar 2024 10:09:25 +0100
Subject: [PATCH v36 1/2] libpq: Add encrypted and non-blocking query
cancellation routines
The existing PQcancel API uses blocking IO, which makes PQcancel
impossible to use in an event loop based codebase without blocking the
event loop until the call returns. It also doesn't encrypt the
connection over which the cancel request is sent, even when the original
connection required encryption.
This commit adds a PQcancelConn struct and assorted functions, which
provide a better mechanism of sending cancel requests; in particular all
the encryption used in the original connection are also used in the
cancel connection. The main entry points are:
- PQcancelCreate creates the PQcancelConn based on the original
connection (but does not establish an actual connection).
- PQcancelStart can be used to initiate non-blocking cancel requests,
using encryption if the original connection did so, which must be
pumped using
- PQcancelPoll.
- PQcancelReset puts a PQcancelConn back in state so that it can be
reused to send a new cancel request to the same connection.
- PQcancelBlocking is a simpler-to-use blocking API that still uses
encryption.
Additional functions are
- PQcancelStatus, mimicks PQstatus;
- PQcancelSocket, mimicks PQcancelSocket;
- PQcancelErrorMessage, mimicks PQerrorMessage;
- PQcancelFinish, mimicks PQfinish.
Author: Jelte Fennema-Nio <[email protected]>
Reviewed-by: Denis Laxalde <[email protected]>
Discussion: https://postgr.es/m/AM5PR83MB0178D3B31CA1B6EC4A8ECC42F7529@AM5PR83MB0178.EURPRD83.prod.outlook.com
---
doc/src/sgml/libpq.sgml | 461 ++++++++++++++++--
src/interfaces/libpq/exports.txt | 9 +
src/interfaces/libpq/fe-cancel.c | 299 +++++++++++-
src/interfaces/libpq/fe-connect.c | 129 ++++-
src/interfaces/libpq/libpq-fe.h | 31 +-
src/interfaces/libpq/libpq-int.h | 5 +
.../modules/libpq_pipeline/libpq_pipeline.c | 125 +++++
src/tools/pgindent/typedefs.list | 1 +
8 files changed, 1014 insertions(+), 46 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index a2bbf33d029..373d0dc3223 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -265,7 +265,7 @@ PGconn *PQsetdb(char *pghost,
<varlistentry id="libpq-PQconnectStartParams">
<term><function>PQconnectStartParams</function><indexterm><primary>PQconnectStartParams</primary></indexterm></term>
<term><function>PQconnectStart</function><indexterm><primary>PQconnectStart</primary></indexterm></term>
- <term><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
+ <term id="libpq-PQconnectPoll"><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
<listitem>
<para>
<indexterm><primary>nonblocking connection</primary></indexterm>
@@ -5287,7 +5287,7 @@ int PQisBusy(PGconn *conn);
<xref linkend="libpq-PQsendQuery"/>/<xref linkend="libpq-PQgetResult"/>
can also attempt to cancel a command that is still being processed
by the server; see <xref linkend="libpq-cancel"/>. But regardless of
- the return value of <xref linkend="libpq-PQcancel"/>, the application
+ the return value of <xref linkend="libpq-PQcancelBlocking"/>, the application
must continue with the normal result-reading sequence using
<xref linkend="libpq-PQgetResult"/>. A successful cancellation will
simply cause the command to terminate sooner than it would have
@@ -6034,10 +6034,402 @@ int PQsetSingleRowMode(PGconn *conn);
<secondary>SQL command</secondary>
</indexterm>
- <para>
- A client application can request cancellation of a command that is
- still being processed by the server, using the functions described in
- this section.
+ <sect2 id="libpq-cancel-conn">
+ <title>Functions for Sending Cancel Requests</title>
+ <variablelist>
+ <varlistentry id="libpq-PQcancelCreate">
+ <term><function>PQcancelCreate</function><indexterm><primary>PQcancelCreate</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Prepares a connection over which a cancel request can be sent.
+<synopsis>
+PGcancelConn *PQcancelCreate(PGconn *conn);
+</synopsis>
+ </para>
+
+ <para>
+ <xref linkend="libpq-PQcancelCreate"/> creates a
+ <structname>PGcancelConn</structname><indexterm><primary>PGcancelConn</primary></indexterm>
+ object, but it won't instantly start sending a cancel request over this
+ connection. A cancel request can be sent over this connection in a
+ blocking manner using <xref linkend="libpq-PQcancelBlocking"/> and in a
+ non-blocking manner using <xref linkend="libpq-PQcancelStart"/>.
+ The return value can be passed to <xref linkend="libpq-PQcancelStatus"/>
+ to check if the <structname>PGcancelConn</structname> object was
+ created successfully. The <structname>PGcancelConn</structname> object
+ is an opaque structure that is not meant to be accessed directly by the
+ application. This <structname>PGcancelConn</structname> object can be
+ used to cancel the query that's running on the original connection in a
+ thread-safe way.
+ </para>
+
+ <para>
+ Many connection parameters of the original client will be reused when
+ setting up the connection for the cancel request. Importantly, if the
+ original connection requires encryption of the connection and/or
+ verification of the target host (using <literal>sslmode</literal> or
+ <literal>gssencmode</literal>), then the connection for the cancel
+ request is made with these same requirements. Any connection options
+ that are only used during authentication or after authentication of the
+ client are ignored though, because cancellation requests do not require
+ authentication and the connection is closed right after the cancellation
+ request is submitted.
+ </para>
+
+ <para>
+ Note that when <function>PQcancelCreate</function> returns a non-null
+ pointer, you must call <xref linkend="libpq-PQcancelFinish"/> when you
+ are finished with it, in order to dispose of the structure and any
+ associated memory blocks. This must be done even if the cancel request
+ failed or was abandoned.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelBlocking">
+ <term><function>PQcancelBlocking</function><indexterm><primary>PQcancelBlocking</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Requests that the server abandons processing of the current command in a blocking manner.
+<synopsis>
+int PQcancelBlocking(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The request is made over the given <structname>PGcancelConn</structname>,
+ which needs to be created with <xref linkend="libpq-PQcancelCreate"/>.
+ The return value of <xref linkend="libpq-PQcancelBlocking"/>
+ is 1 if the cancel request was successfully
+ dispatched and 0 if not. If it was unsuccessful, the error message can be
+ retrieved using <xref linkend="libpq-PQcancelErrorMessage"/>.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelStart">
+ <term><function>PQcancelStart</function><indexterm><primary>PQcancelStart</primary></indexterm></term>
+ <term id="libpq-PQcancelPoll"><function>PQcancelPoll</function><indexterm><primary>PQcancelPoll</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Requests that the server abandons processing of the current command in a non-blocking manner.
+<synopsis>
+int PQcancelStart(PGcancelConn *cancelConn);
+
+PostgresPollingStatusType PQcancelPoll(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The request is made over the given <structname>PGcancelConn</structname>,
+ which needs to be created with <xref linkend="libpq-PQcancelCreate"/>.
+ The return value of <xref linkend="libpq-PQcancelStart"/>
+ is 1 if the cancellation request could be started and 0 if not.
+ If it was unsuccessful, the error message can be
+ retrieved using <xref linkend="libpq-PQcancelErrorMessage"/>.
+ </para>
+
+ <para>
+ If <function>PQcancelStart</function> succeeds, the next stage
+ is to poll <application>libpq</application> so that it can proceed with
+ the cancel connection sequence.
+ Use <xref linkend="libpq-PQcancelSocket"/> to obtain the descriptor of the
+ socket underlying the database connection.
+ (Caution: do not assume that the socket remains the same
+ across <function>PQcancelPoll</function> calls.)
+ Loop thus: If <function>PQcancelPoll(cancelConn)</function> last returned
+ <symbol>PGRES_POLLING_READING</symbol>, wait until the socket is ready to
+ read (as indicated by <function>select()</function>, <function>poll()</function>, or
+ similar system function).
+ Then call <function>PQcancelPoll(cancelConn)</function> again.
+ Conversely, if <function>PQcancelPoll(cancelConn)</function> last returned
+ <symbol>PGRES_POLLING_WRITING</symbol>, wait until the socket is ready
+ to write, then call <function>PQcancelPoll(cancelConn)</function> again.
+ On the first iteration, i.e., if you have yet to call
+ <function>PQcancelPoll(cancelConn)</function>, behave as if it last returned
+ <symbol>PGRES_POLLING_WRITING</symbol>. Continue this loop until
+ <function>PQcancelPoll(cancelConn)</function> returns
+ <symbol>PGRES_POLLING_FAILED</symbol>, indicating the connection procedure
+ has failed, or <symbol>PGRES_POLLING_OK</symbol>, indicating cancel
+ request was successfully dispatched.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ <para>
+ At any time during connection, the status of the connection can be
+ checked by calling <xref linkend="libpq-PQcancelStatus"/>. If this call returns <symbol>CONNECTION_BAD</symbol>, then the
+ cancel procedure has failed; if the call returns <function>CONNECTION_OK</function>, then cancel request was successfully dispatched. Both of these states are equally detectable
+ from the return value of <function>PQcancelPoll</function>, described above. Other states might also occur
+ during (and only during) an asynchronous connection procedure. These
+ indicate the current stage of the connection procedure and might be useful
+ to provide feedback to the user for example. These statuses are:
+
+ <variablelist>
+ <varlistentry id="libpq-connection-allocated">
+ <term><symbol>CONNECTION_ALLOCATED</symbol></term>
+ <listitem>
+ <para>
+ Waiting for a call to <xref linkend="libpq-PQcancelStart"/> or
+ <xref linkend="libpq-PQcancelBlocking"/>, to actually open the
+ socket. This is the connection state right after
+ calling <xref linkend="libpq-PQcancelCreate"/>
+ or <xref linkend="libpq-PQcancelReset"/>. No connection to the
+ server has been initiated yet at this point. To actually start
+ sending the cancel request use <xref linkend="libpq-PQcancelStart"/> or
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-started">
+ <term><symbol>CONNECTION_STARTED</symbol></term>
+ <listitem>
+ <para>
+ Waiting for connection to be made.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-made">
+ <term><symbol>CONNECTION_MADE</symbol></term>
+ <listitem>
+ <para>
+ Connection OK; waiting to send.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-awaiting-response">
+ <term><symbol>CONNECTION_AWAITING_RESPONSE</symbol></term>
+ <listitem>
+ <para>
+ Waiting for a response from the server.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-ssl-startup">
+ <term><symbol>CONNECTION_SSL_STARTUP</symbol></term>
+ <listitem>
+ <para>
+ Negotiating SSL encryption.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-cancel-connection-gss-startup">
+ <term><symbol>CONNECTION_GSS_STARTUP</symbol></term>
+ <listitem>
+ <para>
+ Negotiating GSS encryption.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ Note that, although these constants will remain (in order to maintain
+ compatibility), an application should never rely upon these occurring in a
+ particular order, or at all, or on the status always being one of these
+ documented values. An application might do something like this:
+<programlisting>
+switch(PQcancelStatus(conn))
+{
+ case CONNECTION_STARTED:
+ feedback = "Connecting...";
+ break;
+
+ case CONNECTION_MADE:
+ feedback = "Connected to server...";
+ break;
+.
+.
+.
+ default:
+ feedback = "Connecting...";
+}
+</programlisting>
+ </para>
+
+ <para>
+ The <literal>connect_timeout</literal> connection parameter is ignored
+ when using <function>PQcancelPoll</function>; it is the application's
+ responsibility to decide whether an excessive amount of time has elapsed.
+ Otherwise, <function>PQcancelStart</function> followed by a
+ <function>PQcancelPoll</function> loop is equivalent to
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelStatus">
+ <term><function>PQcancelStatus</function><indexterm><primary>PQcancelStatus</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Returns the status of the cancel connection.
+<synopsis>
+ConnStatusType PQcancelStatus(const PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ The status can be one of a number of values. However, only three of
+ these are seen outside of an asynchronous cancel procedure:
+ <literal>CONNECTION_ALLOCATED</literal>,
+ <literal>CONNECTION_OK</literal> and
+ <literal>CONNECTION_BAD</literal>. The initial state of a
+ <function>PGcancelConn</function> that's successfully created using
+ <xref linkend="libpq-PQcancelCreate"/> is <literal>CONNECTION_ALLOCATED</literal>.
+ A cancel request that was successfully dispatched
+ has the status <literal>CONNECTION_OK</literal>. A failed
+ cancel attempt is signaled by status
+ <literal>CONNECTION_BAD</literal>. An OK status will
+ remain so until <xref linkend="libpq-PQcancelFinish"/> or
+ <xref linkend="libpq-PQcancelReset"/> is called.
+ </para>
+
+ <para>
+ See the entry for <xref linkend="libpq-PQcancelStart"/> and <xref
+ linkend="libpq-PQcancelPoll"/> with regards to other status codes that
+ might be returned.
+ </para>
+
+ <para>
+ Successful dispatch of the cancellation is no guarantee that the request
+ will have any effect, however. If the cancellation is effective, the
+ command being canceled will terminate early and return an error result.
+ If the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at all.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelSocket">
+ <term><function>PQcancelSocket</function><indexterm><primary>PQcancelSocket</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Obtains the file descriptor number of the cancel connection socket to
+ the server. A valid descriptor will be greater than or equal
+ to 0; a result of -1 indicates that no server connection is
+ currently open. This might change as a result of calling all of the
+ functions in this section on the (except for
+ <xref linkend="libpq-PQcancelErrorMessage"/> and
+ <function>PQcancelSocket</function> itself).
+<synopsis>
+int PQcancelSocket(const PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelErrorMessage">
+ <term><function>PQcancelErrorMessage</function><indexterm><primary>PQcancelErrorMessage</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ <indexterm><primary>error message</primary></indexterm> Returns the error message
+ most recently generated by an operation on the cancel connection.
+<synopsis>
+char *PQcancelErrorMessage(const PGcancelConn *cancelconn);
+</synopsis>
+ </para>
+
+ <para>
+ Nearly all <application>libpq</application> functions that take a
+ <structname>PGcancelConn</structname> will set a message for
+ <xref linkend="libpq-PQcancelErrorMessage"/> if they fail. Note that by
+ <application>libpq</application> convention, a nonempty
+ <xref linkend="libpq-PQcancelErrorMessage"/> result can consist of multiple lines,
+ and will include a trailing newline. The caller should not free
+ the result directly. It will be freed when the associated
+ <structname>PGcancelConn</structname> handle is passed to
+ <xref linkend="libpq-PQcancelFinish"/>. The result string should not be
+ expected to remain the same across operations on the
+ <literal>PGcancelConn</literal> structure.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelFinish">
+ <term><function>PQcancelFinish</function><indexterm><primary>PQcancelFinish</primary></indexterm></term>
+ <listitem>
+ <para>
+ Closes the cancel connection (if it did not finish sending the cancel
+ request yet). Also frees memory used by the <structname>PGcancelConn</structname>
+ object.
+<synopsis>
+void PQcancelFinish(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ Note that even if the cancel attempt fails (as
+ indicated by <xref linkend="libpq-PQcancelStatus"/>), the application should call <xref linkend="libpq-PQcancelFinish"/>
+ to free the memory used by the <structname>PGcancelConn</structname> object.
+ The <structname>PGcancelConn</structname> pointer must not be used again after
+ <xref linkend="libpq-PQcancelFinish"/> has been called.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQcancelReset">
+ <term><function>PQcancelReset</function><indexterm><primary>PQcancelReset</primary></indexterm></term>
+ <listitem>
+ <para>
+ Resets the <symbol>PGcancelConn</symbol> so it can be reused for a new
+ cancel connection.
+<synopsis>
+void PQcancelReset(PGcancelConn *cancelConn);
+</synopsis>
+ </para>
+
+ <para>
+ If the <symbol>PGcancelConn</symbol> is currently used to send a cancel
+ request, then this connection is closed. It will then prepare the
+ <symbol>PGcancelConn</symbol> object such that it can be used to send a
+ new cancel request. This can be used to create one <symbol>PGcancelConn</symbol>
+ for a <symbol>PGconn</symbol> and reuse that multiple times throughout
+ the lifetime of the original <symbol>PGconn</symbol>.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </sect2>
+
+ <sect2 id="libpq-cancel-deprecated">
+ <title>Obsolete Functions for Sending Cancel Requests</title>
+
+ <para>
+ These functions represent older methods of sending cancel requests.
+ Although they still work, they are deprecated due to not sending the cancel
+ requests in an encrypted manner, even when the original connection
+ specified <literal>sslmode</literal> or <literal>gssencmode</literal> to
+ require encryption. Thus these older methods are heavily discouraged from
+ being used in new code, and it is recommended to change existing code to
+ use the new functions instead.
+ </para>
<variablelist>
<varlistentry id="libpq-PQgetCancel">
@@ -6046,7 +6438,7 @@ int PQsetSingleRowMode(PGconn *conn);
<listitem>
<para>
Creates a data structure containing the information needed to cancel
- a command issued through a particular database connection.
+ a command using <xref linkend="libpq-PQcancel"/>.
<synopsis>
PGcancel *PQgetCancel(PGconn *conn);
</synopsis>
@@ -6088,36 +6480,37 @@ void PQfreeCancel(PGcancel *cancel);
<listitem>
<para>
- Requests that the server abandon processing of the current command.
-<synopsis>
+ <xref linkend="libpq-PQrequestCancel"/> is a deprecated and insecure
+ variant of <xref linkend="libpq-PQcancelBlocking"/>, but one that can be
+ used safely from within a signal handler. <synopsis>
int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
</synopsis>
</para>
<para>
- The return value is 1 if the cancel request was successfully
- dispatched and 0 if not. If not, <parameter>errbuf</parameter> is filled
- with an explanatory error message. <parameter>errbuf</parameter>
- must be a char array of size <parameter>errbufsize</parameter> (the
- recommended size is 256 bytes).
+ <xref linkend="libpq-PQcancel"/> only exists because of backwards
+ compatibility reasons. <xref linkend="libpq-PQcancelBlocking"/> should be
+ used instead. The only benefit that <xref linkend="libpq-PQcancel"/> has
+ is that it can be safely invoked from a signal handler, if the
+ <parameter>errbuf</parameter> is a local variable in the signal handler.
+ However, this is generally not considered a big enough benefit to be
+ worth the security issues that this function has.
</para>
<para>
- Successful dispatch is no guarantee that the request will have
- any effect, however. If the cancellation is effective, the current
- command will terminate early and return an error result. If the
- cancellation fails (say, because the server was already done
- processing the command), then there will be no visible result at
- all.
+ The <structname>PGcancel</structname> object is read-only as far as
+ <xref linkend="libpq-PQcancel"/> is concerned, so it can also be invoked
+ from a thread that is separate from the one manipulating the
+ <structname>PGconn</structname> object.
</para>
<para>
- <xref linkend="libpq-PQcancel"/> can safely be invoked from a signal
- handler, if the <parameter>errbuf</parameter> is a local variable in the
- signal handler. The <structname>PGcancel</structname> object is read-only
- as far as <xref linkend="libpq-PQcancel"/> is concerned, so it can
- also be invoked from a thread that is separate from the one
- manipulating the <structname>PGconn</structname> object.
+ The return value of <xref linkend="libpq-PQcancel"/>
+ is 1 if the cancel request was successfully
+ dispatched and 0 if not. If not, <parameter>errbuf</parameter> is filled
+ with an explanatory error message. <parameter>errbuf</parameter>
+ must be a char array of size <parameter>errbufsize</parameter> (the
+ recommended size is 256 bytes).
</para>
</listitem>
</varlistentry>
@@ -6129,13 +6522,21 @@ int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
<listitem>
<para>
- <xref linkend="libpq-PQrequestCancel"/> is a deprecated variant of
- <xref linkend="libpq-PQcancel"/>.
+ <xref linkend="libpq-PQrequestCancel"/> is a deprecated and insecure
+ variant of <xref linkend="libpq-PQcancelBlocking"/>.
<synopsis>
int PQrequestCancel(PGconn *conn);
</synopsis>
</para>
+ <para>
+ <xref linkend="libpq-PQrequestCancel"/> only exists because of backwards
+ compatibility reasons. <xref linkend="libpq-PQcancelBlocking"/> should be
+ used instead. There is no benefit to using
+ <xref linkend="libpq-PQrequestCancel"/> over
+ <xref linkend="libpq-PQcancelBlocking"/>.
+ </para>
+
<para>
Requests that the server abandon processing of the current
command. It operates directly on the
@@ -6150,7 +6551,7 @@ int PQrequestCancel(PGconn *conn);
</listitem>
</varlistentry>
</variablelist>
- </para>
+ </sect2>
</sect1>
@@ -9362,7 +9763,7 @@ int PQisthreadsafe();
The deprecated functions <xref linkend="libpq-PQrequestCancel"/> and
<xref linkend="libpq-PQoidStatus"/> are not thread-safe and should not be
used in multithread programs. <xref linkend="libpq-PQrequestCancel"/>
- can be replaced by <xref linkend="libpq-PQcancel"/>.
+ can be replaced by <xref linkend="libpq-PQcancelBlocking"/>.
<xref linkend="libpq-PQoidStatus"/> can be replaced by
<xref linkend="libpq-PQoidValue"/>.
</para>
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 088592deb16..9fbd3d34074 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -193,3 +193,12 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQchangePassword 192
PQsendPipelineSync 193
+PQcancelBlocking 194
+PQcancelStart 195
+PQcancelCreate 196
+PQcancelPoll 197
+PQcancelStatus 198
+PQcancelSocket 199
+PQcancelErrorMessage 200
+PQcancelReset 201
+PQcancelFinish 202
diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c
index d69b8f9f9f4..4d67cb50e9b 100644
--- a/src/interfaces/libpq/fe-cancel.c
+++ b/src/interfaces/libpq/fe-cancel.c
@@ -22,9 +22,21 @@
#include "port/pg_bswap.h"
+/*
+ * pg_cancel_conn (backing struct for PGcancelConn) is a wrapper around a
+ * PGconn to send cancellations using PQcancelBlocking and PQcancelStart. This
+ * isn't just a typedef because we want the compiler to complain when a PGconn
+ * is passed to a function that expects a PGcancelConn, and vice versa.
+ */
+struct pg_cancel_conn
+{
+ PGconn conn;
+};
+
+
/*
* pg_cancel (backing struct for PGcancel) stores all data necessary to send a
- * cancel request.
+ * cancel request using the deprecated PQcancel function.
*/
struct pg_cancel
{
@@ -41,6 +53,289 @@ struct pg_cancel
};
+/*
+ * PQcancelCreate
+ *
+ * Create and return a PGcancelConn, which can be used to securely cancel a
+ * query on the given connection.
+ *
+ * This requires either following the non-blocking flow through
+ * PQcancelStart() and PQcancelPoll(), or the blocking PQcancelBlocking().
+ */
+PGcancelConn *
+PQcancelCreate(PGconn *conn)
+{
+ PGconn *cancelConn = pqMakeEmptyPGconn();
+ pg_conn_host originalHost;
+
+ if (cancelConn == NULL)
+ return NULL;
+
+ /* Check we have an open connection */
+ if (!conn)
+ {
+ libpq_append_conn_error(cancelConn, "passed connection was NULL");
+ return (PGcancelConn *) cancelConn;
+ }
+
+ if (conn->sock == PGINVALID_SOCKET)
+ {
+ libpq_append_conn_error(cancelConn, "passed connection is not open");
+ return (PGcancelConn *) cancelConn;
+ }
+
+ /*
+ * Indicate that this connection is used to send a cancellation
+ */
+ cancelConn->cancelRequest = true;
+
+ if (!pqCopyPGconn(conn, cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Compute derived options
+ */
+ if (!pqConnectOptions2(cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Copy cancellation token data from the original connnection
+ */
+ cancelConn->be_pid = conn->be_pid;
+ cancelConn->be_key = conn->be_key;
+
+ /*
+ * Cancel requests should not iterate over all possible hosts. The request
+ * needs to be sent to the exact host and address that the original
+ * connection used. So we manually create the host and address arrays with
+ * a single element after freeing the host array that we generated from
+ * the connection options.
+ */
+ pqReleaseConnHosts(cancelConn);
+ cancelConn->nconnhost = 1;
+ cancelConn->naddr = 1;
+
+ cancelConn->connhost = calloc(cancelConn->nconnhost, sizeof(pg_conn_host));
+ if (!cancelConn->connhost)
+ goto oom_error;
+
+ originalHost = conn->connhost[conn->whichhost];
+ if (originalHost.host)
+ {
+ cancelConn->connhost[0].host = strdup(originalHost.host);
+ if (!cancelConn->connhost[0].host)
+ goto oom_error;
+ }
+ if (originalHost.hostaddr)
+ {
+ cancelConn->connhost[0].hostaddr = strdup(originalHost.hostaddr);
+ if (!cancelConn->connhost[0].hostaddr)
+ goto oom_error;
+ }
+ if (originalHost.port)
+ {
+ cancelConn->connhost[0].port = strdup(originalHost.port);
+ if (!cancelConn->connhost[0].port)
+ goto oom_error;
+ }
+ if (originalHost.password)
+ {
+ cancelConn->connhost[0].password = strdup(originalHost.password);
+ if (!cancelConn->connhost[0].password)
+ goto oom_error;
+ }
+
+ cancelConn->addr = calloc(cancelConn->naddr, sizeof(AddrInfo));
+ if (!cancelConn->connhost)
+ goto oom_error;
+
+ cancelConn->addr[0].addr = conn->raddr;
+ cancelConn->addr[0].family = conn->raddr.addr.ss_family;
+
+ cancelConn->status = CONNECTION_ALLOCATED;
+ return (PGcancelConn *) cancelConn;
+
+oom_error:
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(cancelConn, "out of memory");
+ return (PGcancelConn *) cancelConn;
+}
+
+
+/*
+ * PQcancelBlocking
+ *
+ * Send a cancellation request in a blocking fashion.
+ * Returns 1 if successful 0 if not.
+ */
+int
+PQcancelBlocking(PGcancelConn *cancelConn)
+{
+ if (!PQcancelStart(cancelConn))
+ return 0;
+ return pqConnectDBComplete(&cancelConn->conn);
+}
+
+/*
+ * PQcancelStart
+ *
+ * Starts sending a cancellation request in a non-blocking fashion. Returns
+ * 1 if successful 0 if not.
+ */
+int
+PQcancelStart(PGcancelConn *cancelConn)
+{
+ if (!cancelConn || cancelConn->conn.status == CONNECTION_BAD)
+ return 0;
+
+ if (cancelConn->conn.status != CONNECTION_ALLOCATED)
+ {
+ libpq_append_conn_error(&cancelConn->conn,
+ "cancel request is already being sent on this connection");
+ cancelConn->conn.status = CONNECTION_BAD;
+ return 0;
+ }
+
+ return pqConnectDBStart(&cancelConn->conn);
+}
+
+/*
+ * PQcancelPoll
+ *
+ * Poll a cancel connection. For usage details see PQconnectPoll.
+ */
+PostgresPollingStatusType
+PQcancelPoll(PGcancelConn *cancelConn)
+{
+ PGconn *conn = &cancelConn->conn;
+ int n;
+
+ /*
+ * We leave most of the connection establishement to PQconnectPoll, since
+ * it's very similar to normal connection establishment. But once we get
+ * to the CONNECTION_AWAITING_RESPONSE we need to start doing our own
+ * thing.
+ */
+ if (conn->status != CONNECTION_AWAITING_RESPONSE)
+ {
+ return PQconnectPoll(conn);
+ }
+
+ /*
+ * At this point we are waiting on the server to close the connection,
+ * which is its way of communicating that the cancel has been handled.
+ */
+
+ n = pqReadData(conn);
+
+ if (n == 0)
+ return PGRES_POLLING_READING;
+
+#ifndef WIN32
+
+ /*
+ * If we receive an error report it, but only if errno is non-zero.
+ * Otherwise we assume it's an EOF, which is what we expect from the
+ * server.
+ *
+ * We skip this for Windows, because Windows is a bit special in its EOF
+ * behaviour for TCP. Sometimes it will error with an ECONNRESET when
+ * there is a clean connection closure. See these threads for details:
+ * https://www.postgresql.org/message-id/flat/90b34057-4176-7bb0-0dbb-9822a5f6425b%40greiz-reinsdorf.de
+ *
+ * https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BOeoETZQ%3DQw5Ub5h3tmwQhBmDA%3DnuNO3KG%3DzWfUypFAw%40mail.gmail.com
+ *
+ * PQcancel ignores such errors and reports success for the cancellation
+ * anyway, so even if this is not always correct we do the same here.
+ */
+ if (n < 0 && errno != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ return PGRES_POLLING_FAILED;
+ }
+#endif
+
+ /*
+ * We don't expect any data, only connection closure. So if we strangely
+ * do receive some data we consider that an error.
+ */
+ if (n > 0)
+ {
+ libpq_append_conn_error(conn, "received unexpected response from server");
+ conn->status = CONNECTION_BAD;
+ return PGRES_POLLING_FAILED;
+ }
+
+ /*
+ * Getting here means that we received an EOF, which is what we were
+ * expecting -- the cancel request has completed.
+ */
+ cancelConn->conn.status = CONNECTION_OK;
+ resetPQExpBuffer(&conn->errorMessage);
+ return PGRES_POLLING_OK;
+}
+
+/*
+ * PQcancelStatus
+ *
+ * Get the status of a cancel connection.
+ */
+ConnStatusType
+PQcancelStatus(const PGcancelConn *cancelConn)
+{
+ return PQstatus(&cancelConn->conn);
+}
+
+/*
+ * PQcancelSocket
+ *
+ * Get the socket of the cancel connection.
+ */
+int
+PQcancelSocket(const PGcancelConn *cancelConn)
+{
+ return PQsocket(&cancelConn->conn);
+}
+
+/*
+ * PQcancelErrorMessage
+ *
+ * Get the socket of the cancel connection.
+ */
+char *
+PQcancelErrorMessage(const PGcancelConn *cancelConn)
+{
+ return PQerrorMessage(&cancelConn->conn);
+}
+
+/*
+ * PQcancelReset
+ *
+ * Resets the cancel connection, so it can be reused to send a new cancel
+ * request.
+ */
+void
+PQcancelReset(PGcancelConn *cancelConn)
+{
+ pqClosePGconn(&cancelConn->conn);
+ cancelConn->conn.status = CONNECTION_ALLOCATED;
+ cancelConn->conn.whichhost = 0;
+ cancelConn->conn.whichaddr = 0;
+ cancelConn->conn.try_next_host = false;
+ cancelConn->conn.try_next_addr = false;
+}
+
+/*
+ * PQcancelFinish
+ *
+ * Closes and frees the cancel connection.
+ */
+void
+PQcancelFinish(PGcancelConn *cancelConn)
+{
+ PQfinish(&cancelConn->conn);
+}
+
/*
* PQgetCancel: get a PGcancel structure corresponding to a connection.
*
@@ -145,7 +440,7 @@ optional_setsockopt(int fd, int protoid, int optid, int value)
/*
- * PQcancel: request query cancel
+ * PQcancel: old, non-encrypted, but signal-safe way of requesting query cancel
*
* The return value is true if the cancel request was successfully
* dispatched, false if not (in which case an error message is available).
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d4e10a0c4f3..8e8634e5baf 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -616,8 +616,17 @@ pqDropServerData(PGconn *conn)
conn->write_failed = false;
free(conn->write_err_msg);
conn->write_err_msg = NULL;
- conn->be_pid = 0;
- conn->be_key = 0;
+
+ /*
+ * Cancel connections need to retain their be_pid and be_key across
+ * PQcancelReset invocations, otherwise they would not have access to the
+ * secret token of the connection they are supposed to cancel.
+ */
+ if (!conn->cancelRequest)
+ {
+ conn->be_pid = 0;
+ conn->be_key = 0;
+ }
}
@@ -923,6 +932,45 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
return true;
}
+/*
+ * Copy over option values from srcConn to dstConn
+ *
+ * Don't put anything cute here --- intelligence should be in
+ * connectOptions2 ...
+ *
+ * Returns true on success. On failure, returns false and sets error message of
+ * dstConn.
+ */
+bool
+pqCopyPGconn(PGconn *srcConn, PGconn *dstConn)
+{
+ const internalPQconninfoOption *option;
+
+ /* copy over connection options */
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ if (option->connofs >= 0)
+ {
+ const char **tmp = (const char **) ((char *) srcConn + option->connofs);
+
+ if (*tmp)
+ {
+ char **dstConnmember = (char **) ((char *) dstConn + option->connofs);
+
+ if (*dstConnmember)
+ free(*dstConnmember);
+ *dstConnmember = strdup(*tmp);
+ if (*dstConnmember == NULL)
+ {
+ libpq_append_conn_error(dstConn, "out of memory");
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+}
+
/*
* connectOptions1
*
@@ -2308,10 +2356,18 @@ pqConnectDBStart(PGconn *conn)
* Set up to try to connect to the first host. (Setting whichhost = -1 is
* a bit of a cheat, but PQconnectPoll will advance it to 0 before
* anything else looks at it.)
+ *
+ * Cancel requests are special though, they should only try one host and
+ * address, and these fields have already been set up in PQcancelCreate, so
+ * leave these fields alone for cancel requests.
*/
- conn->whichhost = -1;
- conn->try_next_addr = false;
- conn->try_next_host = true;
+ if (!conn->cancelRequest)
+ {
+ conn->whichhost = -1;
+ conn->try_next_host = true;
+ conn->try_next_addr = false;
+ }
+
conn->status = CONNECTION_NEEDED;
/* Also reset the target_server_type state if needed */
@@ -2453,7 +2509,10 @@ pqConnectDBComplete(PGconn *conn)
/*
* Now try to advance the state machine.
*/
- flag = PQconnectPoll(conn);
+ if (conn->cancelRequest)
+ flag = PQcancelPoll((PGcancelConn *) conn);
+ else
+ flag = PQconnectPoll(conn);
}
}
@@ -2578,13 +2637,17 @@ keep_going: /* We will come back to here until there is
* Oops, no more hosts.
*
* If we are trying to connect in "prefer-standby" mode, then drop
- * the standby requirement and start over.
+ * the standby requirement and start over. Don't do this for
+ * cancel requests though, since we are certain the list of
+ * servers won't change as the target_server_type option is not
+ * applicable to those connections.
*
* Otherwise, an appropriate error message is already set up, so
* we just need to set the right status.
*/
if (conn->target_server_type == SERVER_TYPE_PREFER_STANDBY &&
- conn->nconnhost > 0)
+ conn->nconnhost > 0 &&
+ !conn->cancelRequest)
{
conn->target_server_type = SERVER_TYPE_PREFER_STANDBY_PASS2;
conn->whichhost = 0;
@@ -3226,6 +3289,29 @@ keep_going: /* We will come back to here until there is
}
#endif /* USE_SSL */
+ /*
+ * For cancel requests this is as far as we need to go in the
+ * connection establishment. Now we can actually send our
+ * cancellation request.
+ */
+ if (conn->cancelRequest)
+ {
+ CancelRequestPacket cancelpacket;
+
+ packetlen = sizeof(cancelpacket);
+ cancelpacket.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
+ cancelpacket.backendPID = pg_hton32(conn->be_pid);
+ cancelpacket.cancelAuthCode = pg_hton32(conn->be_key);
+ if (pqPacketSend(conn, 0, &cancelpacket, packetlen) != STATUS_OK)
+ {
+ libpq_append_conn_error(conn, "could not send cancel packet: %s",
+ SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+ goto error_return;
+ }
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ return PGRES_POLLING_READING;
+ }
+
/*
* Build the startup packet.
*/
@@ -3975,8 +4061,14 @@ keep_going: /* We will come back to here until there is
}
}
- /* We can release the address list now. */
- release_conn_addrinfo(conn);
+ /*
+ * For non cancel requests we can release the address list
+ * now. For cancel requests we never actually resolve
+ * addresses and instead the addrinfo exists for the lifetime
+ * of the connection.
+ */
+ if (!conn->cancelRequest)
+ release_conn_addrinfo(conn);
/*
* Contents of conn->errorMessage are no longer interesting
@@ -4344,6 +4436,7 @@ freePGconn(PGconn *conn)
free(conn->events[i].name);
}
+ release_conn_addrinfo(conn);
pqReleaseConnHosts(conn);
free(conn->client_encoding_initial);
@@ -4495,6 +4588,13 @@ release_conn_addrinfo(PGconn *conn)
static void
sendTerminateConn(PGconn *conn)
{
+ /*
+ * The Postgres cancellation protocol does not have a notion of a
+ * Terminate message, so don't send one.
+ */
+ if (conn->cancelRequest)
+ return;
+
/*
* Note that the protocol doesn't allow us to send Terminate messages
* during the startup phase.
@@ -4548,7 +4648,14 @@ pqClosePGconn(PGconn *conn)
conn->pipelineStatus = PQ_PIPELINE_OFF;
pqClearAsyncResult(conn); /* deallocate result */
pqClearConnErrorState(conn);
- release_conn_addrinfo(conn);
+
+ /*
+ * Release addrinfo, but since cancel requests never change their addrinfo
+ * we don't do that. Otherwise we would have to rebuild it during a
+ * PQcancelReset.
+ */
+ if (!conn->cancelRequest)
+ release_conn_addrinfo(conn);
/* Reset all state obtained from server, too */
pqDropServerData(conn);
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 2c06044a75e..09b485bd2bc 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -79,7 +79,9 @@ typedef enum
CONNECTION_GSS_STARTUP, /* Negotiating GSSAPI. */
CONNECTION_CHECK_TARGET, /* Internal state: checking target server
* properties. */
- CONNECTION_CHECK_STANDBY /* Checking if server is in standby mode. */
+ CONNECTION_CHECK_STANDBY, /* Checking if server is in standby mode. */
+ CONNECTION_ALLOCATED /* Waiting for connection attempt to be
+ * started. */
} ConnStatusType;
typedef enum
@@ -166,6 +168,11 @@ typedef enum
*/
typedef struct pg_conn PGconn;
+/* PGcancelConn encapsulates a cancel connection to the backend.
+ * The contents of this struct are not supposed to be known to applications.
+ */
+typedef struct pg_cancel_conn PGcancelConn;
+
/* PGresult encapsulates the result of a query (or more precisely, of a single
* SQL command --- a query string given to PQsendQuery can contain multiple
* commands and thus return multiple PGresult objects).
@@ -322,16 +329,34 @@ extern PostgresPollingStatusType PQresetPoll(PGconn *conn);
/* Synchronous (blocking) */
extern void PQreset(PGconn *conn);
+/* Create a PGcancelConn that's used to cancel a query on the given PGconn */
+extern PGcancelConn *PQcancelCreate(PGconn *conn);
+
+/* issue a cancel request in a non-blocking manner */
+extern int PQcancelStart(PGcancelConn *cancelConn);
+
+/* issue a blocking cancel request */
+extern int PQcancelBlocking(PGcancelConn *cancelConn);
+
+/* poll a non-blocking cancel request */
+extern PostgresPollingStatusType PQcancelPoll(PGcancelConn *cancelConn);
+extern ConnStatusType PQcancelStatus(const PGcancelConn *cancelConn);
+extern int PQcancelSocket(const PGcancelConn *cancelConn);
+extern char *PQcancelErrorMessage(const PGcancelConn *cancelConn);
+extern void PQcancelReset(PGcancelConn *cancelConn);
+extern void PQcancelFinish(PGcancelConn *cancelConn);
+
+
/* request a cancel structure */
extern PGcancel *PQgetCancel(PGconn *conn);
/* free a cancel structure */
extern void PQfreeCancel(PGcancel *cancel);
-/* issue a cancel request */
+/* deprecated version of PQcancelBlocking, but one which is signal-safe */
extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
-/* backwards compatible version of PQcancel; not thread-safe */
+/* deprecated version of PQcancel; not thread-safe */
extern int PQrequestCancel(PGconn *conn);
/* Accessor functions for PGconn objects */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 3abcd180d6d..9c05f11a6e9 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -409,6 +409,10 @@ struct pg_conn
char *require_auth; /* name of the expected auth method */
char *load_balance_hosts; /* load balance over hosts */
+ bool cancelRequest; /* true if this connection is used to send a
+ * cancel request, instead of being a normal
+ * connection that's used for queries */
+
/* Optional file to write trace info to */
FILE *Pfdebug;
int traceFlags;
@@ -669,6 +673,7 @@ extern void pqClosePGconn(PGconn *conn);
extern int pqPacketSend(PGconn *conn, char pack_type,
const void *buf, size_t buf_len);
extern bool pqGetHomeDirectory(char *buf, int bufsize);
+extern bool pqCopyPGconn(PGconn *srcConn, PGconn *dstConn);
extern bool pqParseIntParam(const char *value, int *result, PGconn *conn,
const char *context);
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index c6c7b1c3a17..a17c97bdaf4 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -215,6 +215,7 @@ static void
test_cancel(PGconn *conn)
{
PGcancel *cancel;
+ PGcancelConn *cancelConn;
PGconn *monitorConn;
char errorbuf[256];
@@ -251,6 +252,130 @@ test_cancel(PGconn *conn)
pg_fatal("failed to run PQrequestCancel: %s", PQerrorMessage(conn));
confirm_query_canceled(conn);
+ /* test PQcancelBlocking */
+ send_cancellable_query(conn, monitorConn);
+ cancelConn = PQcancelCreate(conn);
+ if (!PQcancelBlocking(cancelConn))
+ pg_fatal("failed to run PQcancelBlocking: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+ PQcancelFinish(cancelConn);
+
+ /* test PQcancelCreate and then polling with PQcancelPoll */
+ send_cancellable_query(conn, monitorConn);
+ cancelConn = PQcancelCreate(conn);
+ if (!PQcancelStart(cancelConn))
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_OK)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+
+ /*
+ * test PQcancelReset works on the cancel connection and it can be reused
+ * afterwards
+ */
+ PQcancelReset(cancelConn);
+
+ send_cancellable_query(conn, monitorConn);
+ if (!PQcancelStart(cancelConn))
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_OK)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_canceled(conn);
+
+ PQcancelFinish(cancelConn);
+
fprintf(stderr, "ok\n");
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index a3052a181d1..aa7a25b8f8c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1763,6 +1763,7 @@ PG_Locale_Strategy
PG_Lock_Status
PG_init_t
PGcancel
+PGcancelConn
PGcmdQueueEntry
PGconn
PGdataValue
base-commit: 4945e4ed4a72c3ff41560ccef722c3d70ae07dbb
--
2.34.1
[application/octet-stream] v36-0002-Start-using-new-libpq-cancel-APIs.patch (10.7K, ../../CAGECzQTGqEWO2ER1dh0YHMtyXrmOotQ7K3j5_gE_6VW49iBs0w@mail.gmail.com/3-v36-0002-Start-using-new-libpq-cancel-APIs.patch)
download | inline diff:
From 2164d349aed461bf0a4dc457fdf4b2ee1b333676 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Thu, 14 Dec 2023 13:39:09 +0100
Subject: [PATCH v36 2/2] Start using new libpq cancel APIs
A previous commit introduced new APIs to libpq for cancelling queries.
This replaces the usage of the old APIs in most of the codebase with
these newer ones. This specifically leaves out changes to psql and
pgbench as those would need a much larger refactor to be able to call
them, due to the new functions not being signal-safe.
---
contrib/dblink/dblink.c | 30 +++--
contrib/postgres_fdw/connection.c | 105 +++++++++++++++---
.../postgres_fdw/expected/postgres_fdw.out | 15 +++
contrib/postgres_fdw/sql/postgres_fdw.sql | 7 ++
src/fe_utils/connect_utils.c | 11 +-
src/test/isolation/isolationtester.c | 29 ++---
6 files changed, 145 insertions(+), 52 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 19a362526d2..98dcca3e6fd 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1346,22 +1346,32 @@ PG_FUNCTION_INFO_V1(dblink_cancel_query);
Datum
dblink_cancel_query(PG_FUNCTION_ARGS)
{
- int res;
PGconn *conn;
- PGcancel *cancel;
- char errbuf[256];
+ PGcancelConn *cancelConn;
+ char *msg;
dblink_init();
conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
- cancel = PQgetCancel(conn);
+ cancelConn = PQcancelCreate(conn);
- res = PQcancel(cancel, errbuf, 256);
- PQfreeCancel(cancel);
+ PG_TRY();
+ {
+ if (!PQcancelBlocking(cancelConn))
+ {
+ msg = pchomp(PQcancelErrorMessage(cancelConn));
+ }
+ else
+ {
+ msg = "OK";
+ }
+ }
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancelConn);
+ }
+ PG_END_TRY();
- if (res == 1)
- PG_RETURN_TEXT_P(cstring_to_text("OK"));
- else
- PG_RETURN_TEXT_P(cstring_to_text(errbuf));
+ PG_RETURN_TEXT_P(cstring_to_text(msg));
}
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 4931ebf5915..dcc13dc3b24 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -133,7 +133,7 @@ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue);
static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry);
static void pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel);
static bool pgfdw_cancel_query(PGconn *conn);
-static bool pgfdw_cancel_query_begin(PGconn *conn);
+static bool pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime);
static bool pgfdw_cancel_query_end(PGconn *conn, TimestampTz endtime,
bool consume_input);
static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
@@ -1315,36 +1315,104 @@ pgfdw_cancel_query(PGconn *conn)
endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
CONNECTION_CLEANUP_TIMEOUT);
- if (!pgfdw_cancel_query_begin(conn))
+ if (!pgfdw_cancel_query_begin(conn, endtime))
return false;
return pgfdw_cancel_query_end(conn, endtime, false);
}
static bool
-pgfdw_cancel_query_begin(PGconn *conn)
+pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime)
{
- PGcancel *cancel;
- char errbuf[256];
+ bool timed_out = false;
+ bool failed = false;
+ PGcancelConn *cancel_conn = PQcancelCreate(conn);
- /*
- * Issue cancel request. Unfortunately, there's no good way to limit the
- * amount of time that we might block inside PQgetCancel().
- */
- if ((cancel = PQgetCancel(conn)))
+
+ if (!PQcancelStart(cancel_conn))
{
- if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ PG_TRY();
{
ereport(WARNING,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("could not send cancel request: %s",
- errbuf)));
- PQfreeCancel(cancel);
- return false;
+ pchomp(PQcancelErrorMessage(cancel_conn)))));
}
- PQfreeCancel(cancel);
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancel_conn);
+ }
+ PG_END_TRY();
+ return false;
}
- return true;
+ /* In what follows, do not leak any PGcancelConn on an error. */
+ PG_TRY();
+ {
+ while (true)
+ {
+ TimestampTz now = GetCurrentTimestamp();
+ long cur_timeout;
+ PostgresPollingStatusType pollres = PQcancelPoll(cancel_conn);
+ int waitEvents = WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH;
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ /* If timeout has expired, give up, else get sleep time. */
+ cur_timeout = TimestampDifferenceMilliseconds(now, endtime);
+ if (cur_timeout <= 0)
+ {
+ timed_out = true;
+ failed = true;
+ goto exit;
+ }
+
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ waitEvents |= WL_SOCKET_READABLE;
+ break;
+ case PGRES_POLLING_WRITING:
+ waitEvents |= WL_SOCKET_WRITEABLE;
+ break;
+ default:
+ failed = true;
+ goto exit;
+ }
+
+ /* Sleep until there's something to do */
+ WaitLatchOrSocket(MyLatch, waitEvents, PQcancelSocket(cancel_conn),
+ cur_timeout, PG_WAIT_EXTENSION);
+ ResetLatch(MyLatch);
+
+ CHECK_FOR_INTERRUPTS();
+ }
+exit: ;
+ if (failed)
+ {
+ if (timed_out)
+ {
+ ereport(WARNING,
+ (errmsg("could not cancel request due to timeout")));
+ }
+ else
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not send cancel request: %s",
+ pchomp(PQcancelErrorMessage(cancel_conn)))));
+ }
+ }
+ }
+ PG_FINALLY();
+ {
+ PQcancelFinish(cancel_conn);
+ }
+ PG_END_TRY();
+
+ return !failed;
}
static bool
@@ -1685,7 +1753,10 @@ pgfdw_abort_cleanup_begin(ConnCacheEntry *entry, bool toplevel,
*/
if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
{
- if (!pgfdw_cancel_query_begin(entry->conn))
+ TimestampTz endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
+ CONNECTION_CLEANUP_TIMEOUT);
+
+ if (!pgfdw_cancel_query_begin(entry->conn, endtime))
return false; /* Unable to cancel running query */
*cancel_requested = lappend(*cancel_requested, entry);
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 58a603ac56f..e03160bd975 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2739,6 +2739,21 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
(10 rows)
ALTER VIEW v4 OWNER TO regress_view_owner;
+-- Make sure this big CROSS JOIN query is pushed down
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: (count(*))
+ Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5))
+ Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE))
+(4 rows)
+
+-- Make sure query cancellation works
+SET statement_timeout = '10ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+ERROR: canceling statement due to statement timeout
+RESET statement_timeout;
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e3d147de6da..2626e68cc69 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -737,6 +737,13 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10;
ALTER VIEW v4 OWNER TO regress_view_owner;
+-- Make sure this big CROSS JOIN query is pushed down
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+-- Make sure query cancellation works
+SET statement_timeout = '10ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+RESET statement_timeout;
+
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
diff --git a/src/fe_utils/connect_utils.c b/src/fe_utils/connect_utils.c
index 808d54461fd..5ed9f3ba17b 100644
--- a/src/fe_utils/connect_utils.c
+++ b/src/fe_utils/connect_utils.c
@@ -157,19 +157,14 @@ connectMaintenanceDatabase(ConnParams *cparams,
void
disconnectDatabase(PGconn *conn)
{
- char errbuf[256];
-
Assert(conn != NULL);
if (PQtransactionStatus(conn) == PQTRANS_ACTIVE)
{
- PGcancel *cancel;
+ PGcancelConn *cancelConn = PQcancelCreate(conn);
- if ((cancel = PQgetCancel(conn)))
- {
- (void) PQcancel(cancel, errbuf, sizeof(errbuf));
- PQfreeCancel(cancel);
- }
+ (void) PQcancelBlocking(cancelConn);
+ PQcancelFinish(cancelConn);
}
PQfinish(conn);
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index ed110f740f1..0b342b5c2bb 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -946,26 +946,21 @@ try_complete_step(TestSpec *testspec, PermutationStep *pstep, int flags)
*/
if (td > max_step_wait && !canceled)
{
- PGcancel *cancel = PQgetCancel(conn);
+ PGcancelConn *cancel_conn = PQcancelCreate(conn);
- if (cancel != NULL)
+ if (PQcancelBlocking(cancel_conn))
{
- char buf[256];
-
- if (PQcancel(cancel, buf, sizeof(buf)))
- {
- /*
- * print to stdout not stderr, as this should appear
- * in the test case's results
- */
- printf("isolationtester: canceling step %s after %d seconds\n",
- step->name, (int) (td / USECS_PER_SEC));
- canceled = true;
- }
- else
- fprintf(stderr, "PQcancel failed: %s\n", buf);
- PQfreeCancel(cancel);
+ /*
+ * print to stdout not stderr, as this should appear in
+ * the test case's results
+ */
+ printf("isolationtester: canceling step %s after %d seconds\n",
+ step->name, (int) (td / USECS_PER_SEC));
+ canceled = true;
}
+ else
+ fprintf(stderr, "PQcancel failed: %s\n", PQcancelErrorMessage(cancel_conn));
+ PQcancelFinish(cancel_conn);
}
/*
--
2.34.1
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2024-03-12 16:50 Alvaro Herrera <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
1 sibling, 0 replies; 10+ messages in thread
From: Alvaro Herrera @ 2024-03-12 16:50 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Denis Laxalde <[email protected]>; vignesh C <[email protected]>; Thomas Munro <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; Gregory Stark (as CFM) <[email protected]>; Jelte Fennema <[email protected]>; Daniel Gustafsson <[email protected]>; Peter Eisentraut <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Robert Haas <[email protected]>; [email protected] <[email protected]>
On 2024-Mar-12, Jelte Fennema-Nio wrote:
> On Tue, 12 Mar 2024 at 10:19, Alvaro Herrera <[email protected]> wrote:
> > Here's a last one for the cfbot.
>
> Thanks for committing the first 3 patches btw. Attached a tiny change
> to 0001, which adds "(backing struct for PGcancelConn)" to the comment
> on pg_cancel_conn.
Thanks, I included it. I hope there were no other changes, because I
didn't verify :-) but if there were, please let me know to incorporate
them.
I made a number of other small changes, mostly to the documentation,
nothing fundamental. (Someday we should stop using <listentry> to
document the libpq functions and use refentry's instead ... it'd be
useful to have manpages for these functions.)
One thing I don't like very much is release_conn_addrinfo(), which is
called conditionally in two places but unconditionally in other places.
Maybe it'd make more sense to put this conditionality inside the
function itself, possibly with a "bool force" flag to suppress that in
the cases where it is not desired.
In pqConnectDBComplete, we cast the PGconn * to PGcancelConn * in order
to call PQcancelPoll, which is a bit abusive, but I don't know how to do
better. Maybe we just accept this ... but if PQcancelStart is the only
way to have pqConnectDBStart called from a cancel connection, maybe it'd
be saner to duplicate pqConnectDBStart for cancel conns.
Thanks!
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2024-03-12 17:58 Alvaro Herrera <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Alvaro Herrera @ 2024-03-12 17:58 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Denis Laxalde <[email protected]>; vignesh C <[email protected]>; Thomas Munro <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; Gregory Stark (as CFM) <[email protected]>; Jelte Fennema <[email protected]>; Daniel Gustafsson <[email protected]>; Peter Eisentraut <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Robert Haas <[email protected]>; [email protected] <[email protected]>
Hmm, buildfarm member kestrel (which uses
-fsanitize=undefined,alignment) failed:
# Running: libpq_pipeline -r 700 cancel port=49975 host=/tmp/dFh46H7YGc
dbname='postgres'
test cancellations...
libpq_pipeline:260: query did not fail when it was expected
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=kestrel&dt=2024-03-12%2016%3A41%3A27
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
than society gathers wisdom." (Isaac Asimov)
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-03-12 17:58 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-27 22:52 [PATCH v5 2/2] Improve more insufficient-privileges error messages. Nathan Bossart <[email protected]>
2023-01-27 22:52 [PATCH v4 2/2] Improve more insufficient-privileges error messages. Nathan Bossart <[email protected]>
2023-01-27 22:52 [PATCH v6 2/2] Improve more insufficient-privileges error messages. Nathan Bossart <[email protected]>
2024-03-12 09:19 Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Alvaro Herrera <[email protected]>
2024-03-12 09:53 ` Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Jelte Fennema-Nio <[email protected]>
2024-03-12 11:41 ` Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Jelte Fennema-Nio <[email protected]>
2024-03-12 14:04 ` Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Jelte Fennema-Nio <[email protected]>
2024-03-12 15:45 ` Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Jelte Fennema-Nio <[email protected]>
2024-03-12 17:58 ` Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Alvaro Herrera <[email protected]>
2024-03-12 16:50 ` Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Alvaro Herrera <[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