Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perl_parse() return value breaks PostgreSQL pl/perl #16565

Open
p5pRT opened this issue May 23, 2018 · 17 comments
Open

perl_parse() return value breaks PostgreSQL pl/perl #16565

p5pRT opened this issue May 23, 2018 · 17 comments

Comments

@p5pRT
Copy link

p5pRT commented May 23, 2018

Migrated from rt.perl.org#133220 (status was 'open')

Searchable as RT133220$

@p5pRT
Copy link
Author

p5pRT commented May 23, 2018

From myon@debian.org

The change "properly define perl_parse() return value" with git hash
0301e89 breaks PostgreSQL's pl/perl procedural language, because the
return value isn't 0 anymore.

Perl change by Zefram​:
https://perl5.git.perl.org/perl.git/commitdiff/0301e899536a22752f40481d8a1d141b7a7dda82

PostgreSQL problem analysis by Tom Lane​:
https://www.postgresql.org/message-id/23260.1527026547%40sss.pgh.pa.us

Perl analysis by Matt S Trout​:
https://www.nntp.perl.org/group/perl.perl5.porters/2018/05/msg251029.html

Please consider reverting this, or clarify how users should handle the
return value.

Thanks,
Christoph

@p5pRT
Copy link
Author

p5pRT commented May 23, 2018

From @jmdh

Quick side-note in Debian we also notice breakage of barnowl (https://barnowl.mit.edu/) at least in the test suite, which is caused by perl_run returning 256​:

http​://perl.debian.net/rebuild-logs/perl-5.27/barnowl_1.9-4/barnowl_1.9-4+b3_amd64-2018-05-19T10​:29​:01Z.build

Not sure if there is runtime breakage too.

@p5pRT
Copy link
Author

p5pRT commented May 23, 2018

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented May 23, 2018

From @iabyn

On Wed, 23 May 2018 13​:38​:25 -0700, Christoph Berg via RT wrote​:

The change "properly define perl_parse() return value" with git hash
0301e89 breaks PostgreSQL's pl/perl procedural language, because the
return value isn't 0 anymore.

On Wed, May 23, 2018 at 01​:44​:34PM -0700, Dominic Hargreaves via RT wrote​:

Quick side-note in Debian we also notice breakage of barnowl
(https://barnowl.mit.edu/) at least in the test suite, which is caused
by perl_run returning 256​:

Just to clarify something here​:

Zefram's commit changed the behaviour of the return value of both
perl_parse() and perl_run(), but from the various linked-to discussions
and an initial inspection, I think that only the perl_run() change is
potentially problematical (due to the fact that S_run_body() calls
my_exit(0) rather than just returning).

However, this ticket's subject line claims that the issue is with
perl_parse(), which doesn't seem to be the case. I would like
clarification that perl_parse() isn't an issue.

My gut feeling is that the change to perl_run should be reverted, but the
perl_parse() change be kept.

--
"You may not work around any technical limitations in the software"
  -- Windows Vista license

@p5pRT
Copy link
Author

p5pRT commented May 23, 2018

From @jkeenan

On Wed, 23 May 2018 20​:44​:34 GMT, dom wrote​:

Quick side-note in Debian we also notice breakage of barnowl
(https://barnowl.mit.edu/) at least in the test suite, which is caused
by perl_run returning 256​:

http​://perl.debian.net/rebuild-logs/perl-5.27/barnowl_1.9-
4/barnowl_1.9-4+b3_amd64-2018-05-19T10​:29​:01Z.build

Not sure if there is runtime breakage too.

Dom​: Is there any way the tests found in the link you posted above could be incorporated into the core test suite so that, once this problem is addressed, we get early warnings of problems in the future?

Thank you very much.
Jim Keenan
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented May 23, 2018

From @iabyn

On Wed, May 23, 2018 at 02​:36​:52PM -0700, James E Keenan via RT wrote​:

Dom​: Is there any way the tests found in the link you posted above
could be incorporated into the core test suite so that, once this
problem is addressed, we get early warnings of problems in the future?

This bug is related to how the perl interpreter is embedded in, and called
by, a third-party application. So there's nothing directly from those
tests which could be incorporated into the perl distribution.

Instead, we (p5p) would have to code up an embedded interpreter - probably
within XS​::APItest - then test various combos of running to completion or
doing exit(0) / exit(non-zero).

--
A walk of a thousand miles begins with a single step...
then continues for another 1,999,999 or so.

@p5pRT
Copy link
Author

p5pRT commented May 24, 2018

From @df7cb

On Wed, 23 May 2018 14​:35​:34 -0700, davem wrote​:

However, this ticket's subject line claims that the issue is with
perl_parse(), which doesn't seem to be the case. I would like
clarification that perl_parse() isn't an issue.

plperl.c is failing on perl_run in line 860​:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/pl/plperl/plperl.c;hb=HEAD#l860

Sorry for the bad title, that was just me sloppily copying from the git commit message.

@p5pRT
Copy link
Author

p5pRT commented May 24, 2018

From @jkeenan

On Wed, 23 May 2018 21​:35​:34 GMT, davem wrote​:

On Wed, 23 May 2018 13​:38​:25 -0700, Christoph Berg via RT wrote​:

The change "properly define perl_parse() return value" with git hash
0301e89 breaks PostgreSQL's pl/perl procedural language, because the
return value isn't 0 anymore.

On Wed, May 23, 2018 at 01​:44​:34PM -0700, Dominic Hargreaves via RT wrote​:

Quick side-note in Debian we also notice breakage of barnowl
(https://barnowl.mit.edu/) at least in the test suite, which is caused
by perl_run returning 256​:

Just to clarify something here​:

Zefram's commit changed the behaviour of the return value of both
perl_parse() and perl_run(), but from the various linked-to discussions
and an initial inspection, I think that only the perl_run() change is
potentially problematical (due to the fact that S_run_body() calls
my_exit(0) rather than just returning).

However, this ticket's subject line claims that the issue is with
perl_parse(), which doesn't seem to be the case. I would like
clarification that perl_parse() isn't an issue.

My gut feeling is that the change to perl_run should be reverted, but the
perl_parse() change be kept.

On the basis of this comment from Dave Mitchell, I'm designating this ticket as a blocker, meaning that some decision will have to be made on it before 5.28.0 can be released.

Thank you very much.
Jim Keenan

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented May 25, 2018

From @df7cb

Reverting perl_run and leaving perl_parse alone is, I think, what mst was getting at in his email.

@p5pRT
Copy link
Author

p5pRT commented May 25, 2018

From @jkeenan

On Wed, 23 May 2018 21​:35​:34 GMT, davem wrote​:

On Wed, 23 May 2018 13​:38​:25 -0700, Christoph Berg via RT wrote​:

The change "properly define perl_parse() return value" with git hash
0301e89 breaks PostgreSQL's pl/perl procedural language, because the
return value isn't 0 anymore.

On Wed, May 23, 2018 at 01​:44​:34PM -0700, Dominic Hargreaves via RT wrote​:

Quick side-note in Debian we also notice breakage of barnowl
(https://barnowl.mit.edu/) at least in the test suite, which is caused
by perl_run returning 256​:

Just to clarify something here​:

Zefram's commit changed the behaviour of the return value of both
perl_parse() and perl_run(), but from the various linked-to discussions
and an initial inspection, I think that only the perl_run() change is
potentially problematical (due to the fact that S_run_body() calls
my_exit(0) rather than just returning).

However, this ticket's subject line claims that the issue is with
perl_parse(), which doesn't seem to be the case. I would like
clarification that perl_parse() isn't an issue.

My gut feeling is that the change to perl_run should be reverted, but the
perl_parse() change be kept.

Would it be possible to get a branch which we could use for smoke-testing and which the Debian folks could use for 3rd-party application testing?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented May 28, 2018

From @iabyn

On Wed, May 23, 2018 at 10​:35​:18PM +0100, Dave Mitchell wrote​:

My gut feeling is that the change to perl_run should be reverted, but the
perl_parse() change be kept.

Which I've now done in blead with v5.28.0-RC1-15-g9f9606382c.

I've written some some tests, but not pushed them, because they
potentially run the risk of spuriously failing due to platform or build
combos - i.e. I think the revert is solid, but the tests are fragile, and
it's too late into the RC cycle to make the tests solid. I'll add them to
blead in 5.29.

--
Spock (or Data) is fired from his high-ranking position for not being able
to understand the most basic nuances of about one in three sentences that
anyone says to him.
  -- Things That Never Happen in "Star Trek" #19

@p5pRT
Copy link
Author

p5pRT commented Mar 29, 2019

From @khwilliamson

On Mon, 28 May 2018 08​:16​:26 -0700, davem wrote​:

On Wed, May 23, 2018 at 10​:35​:18PM +0100, Dave Mitchell wrote​:

My gut feeling is that the change to perl_run should be reverted, but the
perl_parse() change be kept.

Which I've now done in blead with v5.28.0-RC1-15-g9f9606382c.

I've written some some tests, but not pushed them, because they
potentially run the risk of spuriously failing due to platform or build
combos - i.e. I think the revert is solid, but the tests are fragile, and
it's too late into the RC cycle to make the tests solid. I'll add them to
blead in 5.29.

Did these tests get added?

--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 30, 2019

From @iabyn

On Thu, Mar 28, 2019 at 07​:29​:50PM -0700, Karl Williamson via RT wrote​:

On Mon, 28 May 2018 08​:16​:26 -0700, davem wrote​:

On Wed, May 23, 2018 at 10​:35​:18PM +0100, Dave Mitchell wrote​:

My gut feeling is that the change to perl_run should be reverted, but the
perl_parse() change be kept.

Which I've now done in blead with v5.28.0-RC1-15-g9f9606382c.

I've written some some tests, but not pushed them, because they
potentially run the risk of spuriously failing due to platform or build
combos - i.e. I think the revert is solid, but the tests are fragile, and
it's too late into the RC cycle to make the tests solid. I'll add them to
blead in 5.29.

Did these tests get added?

No. Not yet.

--
My get-up-and-go just got up and went.

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2019

From @iabyn

I've moved this ticket from 5.30.0 blocker to 5.32.0 blocker

@p5pRT
Copy link
Author

p5pRT commented May 31, 2019

From @jkeenan

On Mon, 28 May 2018 15​:16​:26 GMT, davem wrote​:

On Wed, May 23, 2018 at 10​:35​:18PM +0100, Dave Mitchell wrote​:

My gut feeling is that the change to perl_run should be reverted, but the
perl_parse() change be kept.

Which I've now done in blead with v5.28.0-RC1-15-g9f9606382c.

I've written some some tests, but not pushed them, because they
potentially run the risk of spuriously failing due to platform or build
combos - i.e. I think the revert is solid, but the tests are fragile, and
it's too late into the RC cycle to make the tests solid. I'll add them to
blead in 5.29.

Since we're early in the perl-5.31 dev cycle, could we get these tests added now?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@toddr toddr added this to the 5.32.0 milestone Oct 25, 2019
@toddr toddr removed the 5.32.0 label Oct 25, 2019
@xsawyerx
Copy link
Member

xsawyerx commented Apr 1, 2020

@iabyn If your fix was added to blead in 5.29, can we close this ticket already? We can open a separate ticket for the tests (with the code branch).

@iabyn
Copy link
Contributor

iabyn commented Apr 7, 2020 via email

@khwilliamson khwilliamson removed this from the 5.32.0 milestone Apr 7, 2020
@xenu xenu removed the Severity Low label Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants