Обсуждение: 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
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
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 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.
Thanks, I think ERROR is redefined in Windows as well for some strange reason.
Dave
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 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. > > 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 20 > >> The 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
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 only
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.
Dave Cramer
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 only > > 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. 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