PL/R regression on windows, but not linux with master.
One of our tests purposely throws an error which returns
"ERROR: R interpreter parse error" on linux
and
"WARNING: R interpreter parse error" on windows.
I'm hoping someone can point me to the code that may be responsible? Was
there a change in the error handling that might be attributed to this?
Dave Cramer
Dave Cramer <davecramer@gmail.com> writes:
One of our tests purposely throws an error which returns
"ERROR: R interpreter parse error" on linux
and
"WARNING: R interpreter parse error" on windows.
That's quite bizarre. What is the actual error level according to
the source code, and where is the error being thrown exactly?
I recall that elog.c has some code to force ERROR up to FATAL or
PANIC in some cases, but it shouldn't ever promote a non-error to
an ERROR.
regards, tom lane
On Sat, 10 Apr 2021 at 20:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Dave Cramer <davecramer@gmail.com> writes:
One of our tests purposely throws an error which returns
"ERROR: R interpreter parse error" on linux
and
"WARNING: R interpreter parse error" on windows.That's quite bizarre. What is the actual error level according to
the source code, and where is the error being thrown exactly?I recall that elog.c has some code to force ERROR up to FATAL or
PANIC in some cases, but it shouldn't ever promote a non-error to
an ERROR.
Well it really is an ERROR, and is being downgraded on windows to WARNING.
I was hoping someone familiar with the code could remember something before
I dig into this tomorrow.
Thanks,
Dave
Show quoted text
regards, tom lane
Dave Cramer <davecramer@gmail.com> writes:
On Sat, 10 Apr 2021 at 20:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
That's quite bizarre. What is the actual error level according to
the source code, and where is the error being thrown exactly?
Well it really is an ERROR, and is being downgraded on windows to WARNING.
That seems quite awful.
However, now that I think about it, the elog.h error-level constants
were renumbered not so long ago. Maybe you've failed to recompile
everything for v14?
regards, tom lane
On Sat, 10 Apr 2021 at 20:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Dave Cramer <davecramer@gmail.com> writes:
On Sat, 10 Apr 2021 at 20:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
That's quite bizarre. What is the actual error level according to
the source code, and where is the error being thrown exactly?Well it really is an ERROR, and is being downgraded on windows to
WARNING.
That seems quite awful.
However, now that I think about it, the elog.h error-level constants
were renumbered not so long ago. Maybe you've failed to recompile
everything for v14?
We see this on a CI with a fresh pull from master.
As I said I will dig into it and figure it out.
Cheers,
Dave
Show quoted text
regards, tom lane
On 4/11/21 2:38 AM, Dave Cramer wrote:
On Sat, 10 Apr 2021 at 20:34, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:Dave Cramer <davecramer@gmail.com <mailto:davecramer@gmail.com>> writes:
On Sat, 10 Apr 2021 at 20:24, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:
That's quite bizarre. What is the actual error level according to
the source code, and where is the error being thrown exactly?Well it really is an ERROR, and is being downgraded on windows to
WARNING.
That seems quite awful.
However, now that I think about it, the elog.h error-level constants
were renumbered not so long ago. Maybe you've failed to recompile
everything for v14?We see this on a CI with a fresh pull from master.
As I said I will dig into it and figure it out.
Well, plr.h does this:
#define WARNING 19
#define ERROR 20
which seems a bit weird, because elog.h does this (since 1f9158ba481):
#define WARNING 19
#define WARNING_CLIENT_ONLY 20
#define ERROR 21
Not sure why this would break Windows but not Linux, though.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sat, 10 Apr 2021 at 20:56, Tomas Vondra <tomas.vondra@enterprisedb.com>
wrote:
On 4/11/21 2:38 AM, Dave Cramer wrote:
On Sat, 10 Apr 2021 at 20:34, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:Dave Cramer <davecramer@gmail.com <mailto:davecramer@gmail.com>>
writes:
On Sat, 10 Apr 2021 at 20:24, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:
That's quite bizarre. What is the actual error level according to
the source code, and where is the error being thrown exactly?Well it really is an ERROR, and is being downgraded on windows to
WARNING.
That seems quite awful.
However, now that I think about it, the elog.h error-level constants
were renumbered not so long ago. Maybe you've failed to recompile
everything for v14?We see this on a CI with a fresh pull from master.
As I said I will dig into it and figure it out.
Well, plr.h does this:
#define WARNING 19
#define ERROR 20which seems a bit weird, because elog.h does this (since 1f9158ba481):
#define WARNING 19
#define WARNING_CLIENT_ONLY 20
#define ERROR 21Not sure why this would break Windows but not Linux, though.
Thanks, I think ERROR is redefined in Windows as well for some strange
reason.
Dave
Show quoted text
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 4/10/21 8:56 PM, Tomas Vondra wrote:
On 4/11/21 2:38 AM, Dave Cramer wrote:
On Sat, 10 Apr 2021 at 20:34, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:Dave Cramer <davecramer@gmail.com <mailto:davecramer@gmail.com>> writes:
On Sat, 10 Apr 2021 at 20:24, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:
That's quite bizarre. What is the actual error level according to
the source code, and where is the error being thrown exactly?Well it really is an ERROR, and is being downgraded on windows to
WARNING.
That seems quite awful.
However, now that I think about it, the elog.h error-level constants
were renumbered not so long ago. Maybe you've failed to recompile
everything for v14?We see this on a CI with a fresh pull from master.
As I said I will dig into it and figure it out.
Well, plr.h does this:
#define WARNING 19
#define ERROR 20which seems a bit weird, because elog.h does this (since 1f9158ba481):
#define WARNING 19
#define WARNING_CLIENT_ONLY 20
#define ERROR 21Not sure why this would break Windows but not Linux, though.
The coding pattern in plr.h looks quite breakable. Instead of hard
coding values like this they should save the value from the postgres
headers in another variable before undefining it and then restore that
value after inclusion of the R headers. That would work across versions
even if we renumber them.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
Well, plr.h does this:
#define WARNING 19
#define ERROR 20
The coding pattern in plr.h looks quite breakable. Instead of hard
coding values like this they should save the value from the postgres
headers in another variable before undefining it and then restore that
value after inclusion of the R headers.
Indeed. elog.h already provides a "PGERROR" macro to use for restoring
the value of ERROR. We have not heard of a need to do anything special
for WARNING though --- maybe that's R-specific?
regards, tom lane
On 4/11/21 10:13 AM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
Well, plr.h does this:
#define WARNING 19
#define ERROR 20The coding pattern in plr.h looks quite breakable.
Meh -- that code has gone 18+ years before breaking.
Indeed. elog.h already provides a "PGERROR" macro to use for restoring
the value of ERROR. We have not heard of a need to do anything special
for WARNING though --- maybe that's R-specific?
R also defines WARNING in its headers. If I remember correctly there are (or at
least were, it *has* been 18+ years since I looked at this particular thing)
some odd differences in the R headers under Windows and Linux.
In any case we would be happy to use "PGERROR".
Would an equivalent "PGWARNING" be something we are open to adding and
back-patching?
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes:
On 4/11/21 10:13 AM, Tom Lane wrote:
Indeed. elog.h already provides a "PGERROR" macro to use for restoring
the value of ERROR. We have not heard of a need to do anything special
for WARNING though --- maybe that's R-specific?
R also defines WARNING in its headers.
Ah.
Would an equivalent "PGWARNING" be something we are open to adding and
back-patching?
It's not real obvious how pl/r could solve this in a reliable way
otherwise, so adding that would be OK with me, but I wonder whether
back-patching is going to help you any. You'd still need to compile
against older headers I should think. So I'd suggest
(1) add PGWARNING in HEAD only
(2) in pl/r, do something like
#ifndef PGWARNING
#define PGWARNING 19
#endif
which should be safe since it's that in all previous supported
versions.
Also, I notice that elog.h is wrapping PGERROR in #ifdef WIN32,
which might be an overly constricted view of when it's helpful.
regards, tom lane
I wrote:
Joe Conway <mail@joeconway.com> writes:
Would an equivalent "PGWARNING" be something we are open to adding and
back-patching?
It's not real obvious how pl/r could solve this in a reliable way
otherwise, so adding that would be OK with me, but I wonder whether
back-patching is going to help you any. You'd still need to compile
against older headers I should think. So I'd suggest
(1) add PGWARNING in HEAD only
Concretely, maybe like the attached?
regards, tom lane
Attachments:
make-PGWARNING-PGERROR-universally-available.patchtext/x-diff; charset=us-ascii; name=make-PGWARNING-PGERROR-universally-available.patchDownload
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 9acb57a27b..f53607e12e 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -40,20 +40,22 @@
#define WARNING 19 /* Warnings. NOTICE is for expected messages
* like implicit sequence creation by SERIAL.
* WARNING is for unexpected messages. */
+#define PGWARNING 19 /* Must equal WARNING; see NOTE below. */
#define WARNING_CLIENT_ONLY 20 /* Warnings to be sent to client as usual, but
* never to the server log. */
#define ERROR 21 /* user error - abort transaction; return to
* known state */
-/* Save ERROR value in PGERROR so it can be restored when Win32 includes
- * modify it. We have to use a constant rather than ERROR because macros
- * are expanded only when referenced outside macros.
- */
-#ifdef WIN32
-#define PGERROR 21
-#endif
+#define PGERROR 21 /* Must equal ERROR; see NOTE below. */
#define FATAL 22 /* fatal error - abort process */
#define PANIC 23 /* take down the other backends with me */
+/*
+ * NOTE: the alternate names PGWARNING and PGERROR are useful for dealing
+ * with third-party headers that make other definitions of WARNING and/or
+ * ERROR. One can, for example, re-define ERROR as PGERROR after including
+ * such a header.
+ */
+
/* macros for representing SQLSTATE strings compactly */
#define PGSIXBIT(ch) (((ch) - '0') & 0x3F)
On Sun, 11 Apr 2021 at 12:43, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Joe Conway <mail@joeconway.com> writes:
Would an equivalent "PGWARNING" be something we are open to adding and
back-patching?It's not real obvious how pl/r could solve this in a reliable way
otherwise, so adding that would be OK with me, but I wonder whether
back-patching is going to help you any. You'd still need to compile
against older headers I should think. So I'd suggest
(1) add PGWARNING in HEAD onlyConcretely, maybe like the attached?
+1 from me.
I especially like the changes to the comments as it's more apparent what
they should be used for.
Dave Cramer
Show quoted text
regards, tom lane
On 4/11/21 12:51 PM, Dave Cramer wrote:
On Sun, 11 Apr 2021 at 12:43, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:I wrote:
Joe Conway <mail@joeconway.com <mailto:mail@joeconway.com>> writes:
Would an equivalent "PGWARNING" be something we are open to adding and
back-patching?It's not real obvious how pl/r could solve this in a reliable way
otherwise, so adding that would be OK with me, but I wonder whether
back-patching is going to help you any. You'd still need to compile
against older headers I should think. So I'd suggest
(1) add PGWARNING in HEAD onlyConcretely, maybe like the attached?
+1 from me.
I especially like the changes to the comments as it's more apparent what they
should be used for.
+1
Looks great to me.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes:
On 4/11/21 12:51 PM, Dave Cramer wrote:
On Sun, 11 Apr 2021 at 12:43, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:Concretely, maybe like the attached?
+1 from me.
I especially like the changes to the comments as it's more apparent what they
should be used for.
+1
Looks great to me.
OK, pushed to HEAD only.
regards, tom lane