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

[Tsunakawa, Takayuki]

From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> TBH, I think you are worried about the wrong thing here.  You could drop
> both of those errdetail calls altogether and be little worse off.  In the
> places where we have errdetail calls like "failed system call was xxx",
> the main point is to show the exact parameters that were given to the system
> call, and neither of these do that.  These errdetail messages would only
> be useful if the identical ereport errmsg might be issued for failures from
> different underlying Windows calls --- but I doubt that's what you're
> intending here.

Yes, that's what I'm intending to do.  To enable the user right "Lock pages in 
memory" on Windows, a few Win32 functions need to be called in turn.


> My problem with these messages is I am not sure what "memory user right"
> means.  Probably that just needs a bit of editing.  But I'd go for something
> like "could not do xxx: error code %lu", and not bother mentioning the system
> call name, unless failing to do so has some impact on whether we could
> understand what happened from a field report of this error message.

For the user, each step of enabling the user right is irrelevant.  It just 
matters to him that that the server could not enable the user right.  OTOH, the 
failed Win32 function may help us to talk with Microsoft to troubleshoot the 
problem.  So I used the same messages in those ereport() calls except for the 
function name to eliminate the translation work.


> (See the "Function Names" item in our message style guidelines for more
> about this issue.  Maybe we need to expand that item some more.)

The style guide does not necessarily require the function parameter values.

https://www.postgresql.org/docs/devel/static/error-style-guide.html

[Quote]
If it really seems necessary, mention the system call in the detail message. 
(In some cases, providing the actual values passed to the system call might be 
appropriate information for the detail message.)

postmaster.c doesn't display parameter values, too.

elog(LOG, "CreateProcess call failed: %m (error code %lu)",
 GetLastError());

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

"Tsunakawa, Takayuki"  writes:
> 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.")));

TBH, I think you are worried about the wrong thing here.  You could drop
both of those errdetail calls altogether and be little worse off.  In the
places where we have errdetail calls like "failed system call was xxx",
the main point is to show the exact parameters that were given to the
system call, and neither of these do that.  These errdetail messages
would only be useful if the identical ereport errmsg might be issued
for failures from different underlying Windows calls --- but I doubt
that's what you're intending here.

My problem with these messages is I am not sure what "memory user right"
means.  Probably that just needs a bit of editing.  But I'd go for
something like "could not do xxx: error code %lu", and not bother
mentioning the system call name, unless failing to do so has some
impact on whether we could understand what happened from a field
report of this error message.

(See the "Function Names" item in our message style guidelines
for more about this issue.  Maybe we need to expand that item
some more.)

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> I find it hard to have an opinion on the matter as a non-translator.
> Why not asking translators directly on pgsql-translators?
> 

I didn't think of pgsql-translators.  I'll ask the same question there.  Thanks.

Anyway, this is also a matter of source code style, and those who commit the 
code live here, so I think I need to hear opinions here, too.

Regards
Takayuki Tsunakawa
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Michael Paquier]

On Mon, Feb 6, 2017 at 2:56 PM, Tsunakawa, Takayuki
 wrote:
> 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?

I find it hard to have an opinion on the matter as a non-translator.
Why not asking translators directly on pgsql-translators?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tsunakawa, Takayuki]

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers