[RFC] Should I embed or parameterize syscall/Win32 function names from error messages?

From: "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>
To: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: [RFC] Should I embed or parameterize syscall/Win32 function names from error messages?
Date: 2017-02-06 05:56:14
Message-ID: 0A3221C70F24FB45833433255569204D1F699CF0@G01JPEXMBYT05
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello, all

Could you give me your opinions on the message style? Recently, I got different comments from Magnus and Alvaro during the review of "Supporting huge_pages on Windows", which is now shifted to CommitFest 2017-3. To be more specific, I'm modifying src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c. This file has existing messages like this:

[Existing message]
ereport(FATAL,
(errmsg("could not create shared memory segment: error code %lu", GetLastError()),
errdetail("Failed system call was CreateFileMapping(size=%zu, name=%s).",
size, szShareMem)));

I added a few ereport() calls that emit the same message except for the Win32 API name. Which of the following do you think is the best? I'd like to follow the majority.

[Option 1]
ereport(elevel,
(errmsg("could not enable Lock pages in memory user right"),
errdetail("Failed system call was %s, error code %lu", "OpenProcessToken", GetLastError())));

[Option 2]
ereport(elevel,
(errmsg("could not enable Lock Pages in Memory user right: error code %lu", GetLastError()),
errdetail("Failed system call was OpenProcessToken.")));

Alvaro thinks that Option 1 is better because it eliminates redundant translation work. Magnus says Option 2 is better because it matches the style of existing messages in the same file.

[Magnus's comment]
this seems to be a new pattern of code -- for other similar cases it
just writes LookupPrivilegeValue inside the patch itself. I'm guessing
the idea was for translatability, but I think it's better we stick to
the existing pattern.

[Alvaro's comment]
There are two reasons for doing things this way. One is that you reduce the chances of a translator making a mistake with the function name (say just a typo, or in egregious cases they may even translate the function name). The other is that if you have many of these messages, you only translate the generic part once instead of having the same message a handful of times, exactly identical but for the function name.
So please do apply that kind of pattern wherever possible. We already have the proposed error message, twice. No need for two more occurrences of it.

I'm rather inclined to choose Option 1 to reduce message translation work. Actually, is the Option 3 the best so that it aligns with the existing messages by putting the error code in the primary message?

[Option 3]
ereport(elevel,
(errmsg("could not enable Lock pages in memory user right: error code %lu", GetLastError()),
errdetail("Failed system call was %s", "OpenProcessToken")));

Regards
Takayuki Tsunakawa

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2017-02-06 06:09:10 Re: [RFC] Should I embed or parameterize syscall/Win32 function names from error messages?
Previous Message David Fetter 2017-02-06 05:50:45 Re: 3D Z-curve spatial index