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

[Bug] 73498 Incorrect SQL generated for pg_copy_to() #2214

Closed
wants to merge 2 commits into
base: PHP-7.0
from

Conversation

7 participants
@duncan3dc
Contributor

duncan3dc commented Nov 20, 2016

Postgres uses the DELIMITER keyword since 7.3 (released in 2002).

All currently supported versions of postgres support DELIMITER so I don't think there's any reason to still use DELIMITERS

The following syntax was used before PostgreSQL version 7.3 and is still supported
https://www.postgresql.org/docs/current/static/sql-copy.html#AEN77789

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Nov 20, 2016

Contributor

Please add a test.

Thanks.

Contributor

weltling commented Nov 20, 2016

Please add a test.

Thanks.

@duncan3dc

This comment has been minimized.

Show comment
Hide comment
@duncan3dc

duncan3dc Nov 20, 2016

Contributor

@weltling Doesn't this test already cover it?

Contributor

duncan3dc commented Nov 20, 2016

@weltling Doesn't this test already cover it?

@KalleZ

This comment has been minimized.

Show comment
Hide comment
@KalleZ

KalleZ Nov 20, 2016

Contributor

@duncan3dc I suppose it does, it is just good to have a test specific for this for each bug fix so it is easier to find any regressions that may occur later on :)

Contributor

KalleZ commented Nov 20, 2016

@duncan3dc I suppose it does, it is just good to have a test specific for this for each bug fix so it is easier to find any regressions that may occur later on :)

@duncan3dc

This comment has been minimized.

Show comment
Hide comment
@duncan3dc

duncan3dc Nov 20, 2016

Contributor

@KalleZ Thinking about it further, I think this change should standalone from the bug. I think something else is causing the bug, and the error message is slightly misleading

Contributor

duncan3dc commented Nov 20, 2016

@KalleZ Thinking about it further, I think this change should standalone from the bug. I think something else is causing the bug, and the error message is slightly misleading

@duncan3dc duncan3dc changed the title from [Bug] 73498 Incorrect SQL generated for pg_copy_to() to Update SQL generated for pg_copy_to() Nov 20, 2016

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Nov 20, 2016

Contributor

@duncan3dc nope, it doesn't. Please see the reproduce snippet in the ticket. The linked test uses a table name directly, that tells the difference.

Thanks.

Contributor

weltling commented Nov 20, 2016

@duncan3dc nope, it doesn't. Please see the reproduce snippet in the ticket. The linked test uses a table name directly, that tells the difference.

Thanks.

@duncan3dc

This comment has been minimized.

Show comment
Hide comment
@duncan3dc

duncan3dc Nov 20, 2016

Contributor

@weltling I've changed this pr now to just be about updating the DELIMITER syntax, I'll take another look at the actual bug when I get a minute

Contributor

duncan3dc commented Nov 20, 2016

@weltling I've changed this pr now to just be about updating the DELIMITER syntax, I'll take another look at the actual bug when I get a minute

@duncan3dc duncan3dc changed the base branch from master to PHP-7.0 Nov 20, 2016

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Nov 20, 2016

Contributor

@duncan3dc but this is still the bugfix to 73498 :) So renaming the PR doesn't sound right. PostgreSQL before 7.3 just doesn't support the sub select syntax. But there are yet two syntax changes in the versions that we support. That's what we need to test, easy done with just modifying the other test.

Thanks.

Contributor

weltling commented Nov 20, 2016

@duncan3dc but this is still the bugfix to 73498 :) So renaming the PR doesn't sound right. PostgreSQL before 7.3 just doesn't support the sub select syntax. But there are yet two syntax changes in the versions that we support. That's what we need to test, easy done with just modifying the other test.

Thanks.

@php-pulls

This comment has been minimized.

Show comment
Hide comment
@php-pulls

php-pulls Nov 21, 2016

Comment on behalf of krakjoe at php.net:

labelling

php-pulls commented Nov 21, 2016

Comment on behalf of krakjoe at php.net:

labelling

@php-pulls php-pulls added the Bugfix label Nov 21, 2016

duncan3dc added some commits Nov 20, 2016

Fix bug #73498
Postgres uses the DELIMITER keyword since 7.3
And WITH is no longer required/used

@duncan3dc duncan3dc changed the title from Update SQL generated for pg_copy_to() to [Bug] 73498 Incorrect SQL generated for pg_copy_to() Nov 21, 2016

@duncan3dc

This comment has been minimized.

Show comment
Hide comment
@duncan3dc

duncan3dc Nov 21, 2016

Contributor

@weltling Sorry for the confusion, thanks for bearing with me. I've added a test and completed the bugfix now

Contributor

duncan3dc commented Nov 21, 2016

@weltling Sorry for the confusion, thanks for bearing with me. I've added a test and completed the bugfix now

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Nov 22, 2016

Contributor

Merged with 644e290, d36d4c7.

Thank you!

Contributor

weltling commented Nov 22, 2016

Merged with 644e290, d36d4c7.

Thank you!

@weltling weltling closed this Nov 22, 2016

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Nov 22, 2016

Contributor

this change effectivly means that support with postgres < 7.3 was dropped, correct? if so, please mention it in the changelog.

Contributor

staabm commented Nov 22, 2016

this change effectivly means that support with postgres < 7.3 was dropped, correct? if so, please mention it in the changelog.

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Nov 22, 2016

Contributor

These PostgreSQL versions are already not supported in PHP, besides they're EOL.

Thanks.

Contributor

weltling commented Nov 22, 2016

These PostgreSQL versions are already not supported in PHP, besides they're EOL.

Thanks.

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Nov 22, 2016

Member

@weltling What is the currently minimal supported version? If pgsql < 7.3 is not supported, there are probably some checks we can drop, e.g. going by config.m4 things like https://github.com/php/php-src/blob/master/ext/pgsql/config.m4#L75.

(It would be nice in general if we could write down minimal versions for all libraries somewhere.)

Member

nikic commented Nov 22, 2016

@weltling What is the currently minimal supported version? If pgsql < 7.3 is not supported, there are probably some checks we can drop, e.g. going by config.m4 things like https://github.com/php/php-src/blob/master/ext/pgsql/config.m4#L75.

(It would be nice in general if we could write down minimal versions for all libraries somewhere.)

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Nov 22, 2016

Contributor

@nikic With PostgreSQL - to my current knowledge, the lowest supported in PHP is 8.1. For PHP 7.0 however, at least 9.3 is required for the proper 64-bit support. I was doing research some time ago because of another bug - even for the older EOL'd CentOS, RPMs for PG < 7.3 are not provided officially anymore https://yum.postgresql.org/repopackages.php . So lower than 7.3 is for sure out of question.

Absolutely, having a list of versions defined would be useful. I used to maintain that wiki page for Windows libs https://wiki.php.net/internals/windows/libs , but now it just screams for update as I'd have to kill a half of a day to actualize it :) Meantime, there's also another approach, that i prepare for the dep update automation new SDK http://windows.php.net/downloads/php-sdk/deps/series/ , so it's more or less tracked. Except we in general try to maintain the wide compat, it seems to be hard. The most exts have no dedicated maintainers, who would know fine details. Nowadays, with a small exception, contributors are not concentrated on wider spectrum. And it's more about fixing issues, than tracking new features and versions :( Some initiative could be started, to maintain such a list globally, i'm just not sure it wouldn't end up in some unmaintained state as well.

Thanks.

Contributor

weltling commented Nov 22, 2016

@nikic With PostgreSQL - to my current knowledge, the lowest supported in PHP is 8.1. For PHP 7.0 however, at least 9.3 is required for the proper 64-bit support. I was doing research some time ago because of another bug - even for the older EOL'd CentOS, RPMs for PG < 7.3 are not provided officially anymore https://yum.postgresql.org/repopackages.php . So lower than 7.3 is for sure out of question.

Absolutely, having a list of versions defined would be useful. I used to maintain that wiki page for Windows libs https://wiki.php.net/internals/windows/libs , but now it just screams for update as I'd have to kill a half of a day to actualize it :) Meantime, there's also another approach, that i prepare for the dep update automation new SDK http://windows.php.net/downloads/php-sdk/deps/series/ , so it's more or less tracked. Except we in general try to maintain the wide compat, it seems to be hard. The most exts have no dedicated maintainers, who would know fine details. Nowadays, with a small exception, contributors are not concentrated on wider spectrum. And it's more about fixing issues, than tracking new features and versions :( Some initiative could be started, to maintain such a list globally, i'm just not sure it wouldn't end up in some unmaintained state as well.

Thanks.

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Nov 22, 2016

Contributor

Typo : "contributors ARE concentrated on wider spectrum" :)

Contributor

weltling commented Nov 22, 2016

Typo : "contributors ARE concentrated on wider spectrum" :)

@bukka

This comment has been minimized.

Show comment
Hide comment
@bukka

bukka Dec 6, 2016

Contributor

@weltling Maybe we could add another row to EXTENSIONS called for example DEPENDENCIES where would be the lowest supported version. Even if it ends up outdated, it will be clear that some versions are not supported so it might useful anyway. Just an idea...

Contributor

bukka commented Dec 6, 2016

@weltling Maybe we could add another row to EXTENSIONS called for example DEPENDENCIES where would be the lowest supported version. Even if it ends up outdated, it will be clear that some versions are not supported so it might useful anyway. Just an idea...

@duncan3dc duncan3dc deleted the duncan3dc:bug-73498 branch Apr 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment