Обсуждение: 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

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