Thread: PL/R regression on windows, but not linux with master.

PL/R regression on windows, but not linux with master.

From
Dave Cramer
Date:
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

Re: PL/R regression on windows, but not linux with master.

From
Tom Lane
Date:
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



Re: PL/R regression on windows, but not linux with master.

From
Dave Cramer
Date:


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

Re: PL/R regression on windows, but not linux with master.

From
Tom Lane
Date:
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



Re: PL/R regression on windows, but not linux with master.

From
Dave Cramer
Date:




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

Re: PL/R regression on windows, but not linux with master.

From
Tomas Vondra
Date:
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



Re: PL/R regression on windows, but not linux with master.

From
Dave Cramer
Date:


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

Re: PL/R regression on windows, but not linux with master.

From
Andrew Dunstan
Date:
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




Re: PL/R regression on windows, but not linux with master.

From
Tom Lane
Date:
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



Re: PL/R regression on windows, but not linux with master.

From
Joe Conway
Date:
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



Re: PL/R regression on windows, but not linux with master.

From
Tom Lane
Date:
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



Re: PL/R regression on windows, but not linux with master.

From
Tom Lane
Date:
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)

Re: PL/R regression on windows, but not linux with master.

From
Dave Cramer
Date:


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

Re: PL/R regression on windows, but not linux with master.

From
Joe Conway
Date:
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



Re: PL/R regression on windows, but not linux with master.

From
Tom Lane
Date:
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