Horton

Thread

Commits

  1. Add macro PGWARNING, and make PGERROR available on all platforms.

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

    Dave Cramer <davecramer@gmail.com> — 2021-04-10T23:53:44Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-11T00:24:04Z

    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

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

    Dave Cramer <davecramer@gmail.com> — 2021-04-11T00:27:53Z

    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
>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-11T00:34:00Z

    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

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

    Dave Cramer <davecramer@gmail.com> — 2021-04-11T00:38:17Z

    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
>

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-04-11T00:56:35Z

    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

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

    Dave Cramer <davecramer@gmail.com> — 2021-04-11T01:11:01Z

    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
>

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

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-11T12:04:51Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-11T14:13:23Z

    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

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

    Joe Conway <mail@joeconway.com> — 2021-04-11T15:01:54Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-11T15:34:02Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-11T16:42:58Z

    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

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

    Dave Cramer <davecramer@gmail.com> — 2021-04-11T16:51:27Z

    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
>
>

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

    Joe Conway <mail@joeconway.com> — 2021-04-11T16:55:08Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-11T17:23:49Z

    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