isolationtester: add session name to application name
Hi,
When writing / debugging an isolation test it's sometimes useful to see which
session holds what lock etc. I find it kind of painful to map pg_stat_activity
/ pg_locks / log output to the isolationtester spec. Sometimes its easy enough
to infer identity based on a statement, but far from all the time.
I found it very helpful to have each session's setup step do something like
SET application_name = 'isolation/prune-recently-dead/vac';
These days isolationtester.c already prefixes log output with the session
name. How about doing the same for application_name? It's a *tad* more
complicated than I'd like because isolationtester.c currently doesn't know the
name of the test its executing.
The attached patch executes
SELECT set_config('application_name', current_setting('application_name') || '/' || $1, false);
when establishing connections to deal with that.
As attached this appends "control connection" for the control connection, but
perhaps we should just not append anything for that?
Greetings,
Andres Freund
Attachments:
0001-isolationtester-append-session-name-to-application_n.patchtext/x-diff; charset=us-asciiDownload
From fce722b66ae2a727c0300b3ece843e49f61e0359 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 10 Dec 2021 17:17:21 -0800
Subject: [PATCH] isolationtester: append session name to application_name.
Author: Andres Freund <andres@anarazel.de>
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
.../expected/insert-conflict-specconflict.out | 20 ++++++-------
src/test/isolation/isolationtester.c | 28 +++++++++++++++++--
.../specs/insert-conflict-specconflict.spec | 5 +---
3 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/src/test/isolation/expected/insert-conflict-specconflict.out b/src/test/isolation/expected/insert-conflict-specconflict.out
index bb8f950f2cf..e34a821c403 100644
--- a/src/test/isolation/expected/insert-conflict-specconflict.out
+++ b/src/test/isolation/expected/insert-conflict-specconflict.out
@@ -490,15 +490,15 @@ step controller_print_speculative_locks:
WHERE
locktype IN ('spectoken', 'transactionid')
AND pa.datname = current_database()
- AND pa.application_name LIKE 'isolation/insert-conflict-specconflict-s%'
+ AND pa.application_name LIKE 'isolation/insert-conflict-specconflict/s%'
ORDER BY 1, 2, 3, 4;
application_name |locktype |mode |granted
-----------------------------------------+-------------+-------------+-------
-isolation/insert-conflict-specconflict-s1|spectoken |ShareLock |f
-isolation/insert-conflict-specconflict-s1|transactionid|ExclusiveLock|t
-isolation/insert-conflict-specconflict-s2|spectoken |ExclusiveLock|t
-isolation/insert-conflict-specconflict-s2|transactionid|ExclusiveLock|t
+isolation/insert-conflict-specconflict/s1|spectoken |ShareLock |f
+isolation/insert-conflict-specconflict/s1|transactionid|ExclusiveLock|t
+isolation/insert-conflict-specconflict/s2|spectoken |ExclusiveLock|t
+isolation/insert-conflict-specconflict/s2|transactionid|ExclusiveLock|t
(4 rows)
step controller_unlock_2_4: SELECT pg_advisory_unlock(2, 4);
@@ -517,14 +517,14 @@ step controller_print_speculative_locks:
WHERE
locktype IN ('spectoken', 'transactionid')
AND pa.datname = current_database()
- AND pa.application_name LIKE 'isolation/insert-conflict-specconflict-s%'
+ AND pa.application_name LIKE 'isolation/insert-conflict-specconflict/s%'
ORDER BY 1, 2, 3, 4;
application_name |locktype |mode |granted
-----------------------------------------+-------------+-------------+-------
-isolation/insert-conflict-specconflict-s1|transactionid|ExclusiveLock|t
-isolation/insert-conflict-specconflict-s1|transactionid|ShareLock |f
-isolation/insert-conflict-specconflict-s2|transactionid|ExclusiveLock|t
+isolation/insert-conflict-specconflict/s1|transactionid|ExclusiveLock|t
+isolation/insert-conflict-specconflict/s1|transactionid|ShareLock |f
+isolation/insert-conflict-specconflict/s2|transactionid|ExclusiveLock|t
(3 rows)
step s2_commit: COMMIT;
@@ -544,7 +544,7 @@ step controller_print_speculative_locks:
WHERE
locktype IN ('spectoken', 'transactionid')
AND pa.datname = current_database()
- AND pa.application_name LIKE 'isolation/insert-conflict-specconflict-s%'
+ AND pa.application_name LIKE 'isolation/insert-conflict-specconflict/s%'
ORDER BY 1, 2, 3, 4;
application_name|locktype|mode|granted
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 88594a3cb5d..c49c0519b32 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -154,10 +154,14 @@ main(int argc, char **argv)
for (i = 0; i < nconns; i++)
{
+ const char *sessionname;
+
if (i == 0)
- conns[i].sessionname = "control connection";
+ sessionname = "control connection";
else
- conns[i].sessionname = testspec->sessions[i - 1]->name;
+ sessionname = testspec->sessions[i - 1]->name;
+
+ conns[i].sessionname = sessionname;
conns[i].conn = PQconnectdb(conninfo);
if (PQstatus(conns[i].conn) != CONNECTION_OK)
@@ -182,6 +186,26 @@ main(int argc, char **argv)
blackholeNoticeProcessor,
NULL);
+ /*
+ * Similarly, append the session name to application_name to make it
+ * easier to map spec file sesions to log output and
+ * pg_stat_activity. The reason to append instead of just setting the
+ * name is that we don't know the name of the test currently running.
+ */
+ res = PQexecParams(conns[i].conn,
+ "SELECT set_config('application_name',\n"
+ " current_setting('application_name') || '/' || $1,\n"
+ " false)",
+ 1, NULL,
+ &sessionname,
+ NULL, NULL, 0);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ fprintf(stderr, "setting of application name failed: %s",
+ PQerrorMessage(conns[i].conn));
+ exit(1);
+ }
+
/* Save each connection's backend PID for subsequent use. */
conns[i].backend_pid = PQbackendPID(conns[i].conn);
conns[i].backend_pid_str = psprintf("%d", conns[i].backend_pid);
diff --git a/src/test/isolation/specs/insert-conflict-specconflict.spec b/src/test/isolation/specs/insert-conflict-specconflict.spec
index 55b8bb100f4..0d55a015b6e 100644
--- a/src/test/isolation/specs/insert-conflict-specconflict.spec
+++ b/src/test/isolation/specs/insert-conflict-specconflict.spec
@@ -47,7 +47,6 @@ session controller
setup
{
SET default_transaction_isolation = 'read committed';
- SET application_name = 'isolation/insert-conflict-specconflict-controller';
}
step controller_locks {SELECT pg_advisory_lock(sess, lock), sess, lock FROM generate_series(1, 2) a(sess), generate_series(1,3) b(lock);}
step controller_unlock_1_1 { SELECT pg_advisory_unlock(1, 1); }
@@ -66,7 +65,7 @@ step controller_print_speculative_locks {
WHERE
locktype IN ('spectoken', 'transactionid')
AND pa.datname = current_database()
- AND pa.application_name LIKE 'isolation/insert-conflict-specconflict-s%'
+ AND pa.application_name LIKE 'isolation/insert-conflict-specconflict/s%'
ORDER BY 1, 2, 3, 4;
}
@@ -75,7 +74,6 @@ setup
{
SET default_transaction_isolation = 'read committed';
SET spec.session = 1;
- SET application_name = 'isolation/insert-conflict-specconflict-s1';
}
step s1_begin { BEGIN; }
step s1_create_non_unique_index { CREATE INDEX upserttest_key_idx ON upserttest((blurt_and_lock_4(key))); }
@@ -90,7 +88,6 @@ setup
{
SET default_transaction_isolation = 'read committed';
SET spec.session = 2;
- SET application_name = 'isolation/insert-conflict-specconflict-s2';
}
step s2_begin { BEGIN; }
step s2_upsert { INSERT INTO upserttest(key, data) VALUES('k1', 'inserted s2') ON CONFLICT (blurt_and_lock_123(key)) DO UPDATE SET data = upserttest.data || ' with conflict update s2'; }
--
2.34.0
On 12/10/21 20:20, Andres Freund wrote:
The attached patch executes
SELECT set_config('application_name', current_setting('application_name') || '/' || $1, false);
when establishing connections to deal with that.
Sounds good
As attached this appends "control connection" for the control connection, but
perhaps we should just not append anything for that?
"control connection" seems reasonable.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Fri, Dec 10, 2021 at 05:20:52PM -0800, Andres Freund wrote:
These days isolationtester.c already prefixes log output with the session
name. How about doing the same for application_name? It's a *tad* more
complicated than I'd like because isolationtester.c currently doesn't know the
name of the test its executing.
+1 for the idea. Maybe it could be backpatched? It could be really
useful to have the same amount of details across all the stable
branches to ease any future backpatch of a test. It does not seem to
me that many people would rely much on application_name in out-of-core
test, but if that's the case such tests would suddenly break after a
the next minor upgrade.
As attached this appends "control connection" for the control connection, but
perhaps we should just not append anything for that?
Keeping "control connection" seems is fine for me for these.
+ * easier to map spec file sesions to log output and
One s/sesions/sessions/ here.
--
Michael
Hi,
On 2021-12-13 19:46:34 +0900, Michael Paquier wrote:
On Fri, Dec 10, 2021 at 05:20:52PM -0800, Andres Freund wrote:
These days isolationtester.c already prefixes log output with the session
name. How about doing the same for application_name? It's a *tad* more
complicated than I'd like because isolationtester.c currently doesn't know the
name of the test its executing.+1 for the idea. Maybe it could be backpatched?
Not entirely trivially - the changes have some dependencies on other changes
(e.g. b1907d688, more on 741d7f104, but that was backpatched). I guess we
could backpatch b1907d688 as well, but I'm not sure its worth it?
+ * easier to map spec file sesions to log output and
One s/sesions/sessions/ here.
Ah, good catch.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2021-12-13 19:46:34 +0900, Michael Paquier wrote:
+1 for the idea. Maybe it could be backpatched?
Not entirely trivially - the changes have some dependencies on other changes
(e.g. b1907d688, more on 741d7f104, but that was backpatched). I guess we
could backpatch b1907d688 as well, but I'm not sure its worth it?
I think we've more recently had the idea that isolationtester features
should be back-patched to avoid gotchas when back-patching test cases.
For instance, all the isolationtester work I did this past summer was
back-patched. So from that vantage point, back-patching b1907d688
seems fine.
regards, tom lane
Hi,
On 2021-12-13 13:57:52 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2021-12-13 19:46:34 +0900, Michael Paquier wrote:
+1 for the idea. Maybe it could be backpatched?
Not entirely trivially - the changes have some dependencies on other changes
(e.g. b1907d688, more on 741d7f104, but that was backpatched). I guess we
could backpatch b1907d688 as well, but I'm not sure its worth it?I think we've more recently had the idea that isolationtester features
should be back-patched to avoid gotchas when back-patching test cases.
For instance, all the isolationtester work I did this past summer was
back-patched. So from that vantage point, back-patching b1907d688
seems fine.
Since there seems support for that approach, I've backpatched b1907d688 and
will push application_name isolationtester change once running the tests
across all branches finishes locally.
Regards,
Andres