Remove the Upsert implementation for PostgreSQL < 9.5 [#3129560]

Problem/Motivation

The minimum required version of PostgreSQL is 10. In the Connection class of the PostgreSQL driver we have the following code:

  public function upsert($table, array $options = []) {
    // Use the (faster) native Upsert implementation for PostgreSQL >= 9.5.
    if (version_compare($this->version(), '9.5', '>=')) {
      $class = $this->getDriverClass('NativeUpsert');
    }
    else {
      $class = $this->getDriverClass('Upsert');
    }

    return new $class($this, $table, $options);
  }

The method is no longer necessary.

Proposed resolution

1. Remove the method Drupal\Core\Database\Driver\pgsql\Connection::upsert().
2. Then remove the class Drupal\Core\Database\Driver\pgsql\Upsert.
3. After that rename the class Drupal\Core\Database\Driver\pgsql\NativeUpsert to Drupal\Core\Database\Driver\pgsql\Upsert.
4. In the renamed class, change the class name from "NativeUpsert" to "Upsert" and remove the link to http://www.postgresql.org/docs/9.5/static/sql-insert.html#SQL-ON-CONFLICT.
5. The file "core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/NativeUpsert.php" can also be removed.

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

Comment	File	Size	Author
#22	the-upsert-class-was-removed-for-PostgreSQL-database-driver-3129560-21.patch	12.58 KB	rik-dev
#16	3129560-5.patch	12.72 KB	rik-dev
#10	3129560-5.patch	12.67 KB	rik-dev
#7	3129560-5.patch	12.67 KB	rik-dev
#5	3129560-5.patch	12.85 KB	rik-dev

Comments

Comment #1

21 April 2020 at 11:48

daffie created an issue. See original summary.

Comment #2

daffie commented 23 June 2020 at 08:14

Issue summary:

View changes

Comment #3

daffie commented 23 June 2020 at 08:19

Issue summary:

View changes

Comment #4

rik-dev commented 23 June 2020 at 09:03

Assigned:

Unassigned

» rik-dev

Comment #5

rik-dev commented 23 June 2020 at 14:58

Status:

Active

» Needs review

Status	File	Size
new	3129560-5.patch	12.85 KB

Comment #6

daffie commented 24 June 2020 at 08:46

Status:

Needs review

» Needs work

Unfortunately the patch does not apply. Therefore I am not able to do a full review. Most of what the patch should do is correct. For now I have only 2 remarks:

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
@@ -2,10 +2,9 @@
-use Drupal\Core\Database\Query\Upsert as QueryUpsert;

This use-statement will still be needed. This class is a subclass of the base Upsert class.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
@@ -2,10 +2,9 @@
- * PostgreSQL implementation of \Drupal\Core\Database\Query\Upsert.
+ * PostgreSQL implementation of native \Drupal\Core\Database\Query\Upsert.

This change is not necessary.

Comment #7

rik-dev commented 24 June 2020 at 09:37

Status	File	Size
new	3129560-5.patch	12.67 KB

@daffie Thanks for took it to look. The patch was updated.

Comment #8

rik-dev commented 24 June 2020 at 09:37

Status:

Needs work

» Needs review

Comment #9

daffie commented 24 June 2020 at 09:51

@rik-dev: Your newest patch does not apply on my local machine. Did you do a "git pull" before creating your patch?

Comment #10

rik-dev commented 24 June 2020 at 10:15

Status	File	Size
new	3129560-5.patch	12.67 KB

2 files were hidden/shown/deleted

Status	File	Size
hidden	3129560-5.patch	12.85 KB
hidden	3129560-5.patch	12.67 KB

@daffie sure, I made "git pull" before patch that. Try this one.

Comment #11

rik-dev commented 24 June 2020 at 10:21

@daffie After renamed the file Drupal\Core\Database\Driver\pgsql\NativeUpsert to Drupal\Core\Database\Driver\pgsql\Upsert I got the same error, what do I do wrong?

Comment #12

daffie commented 24 June 2020 at 10:24

@rik-dev: Are you using the 9.1.x branch?

Comment #13

rik-dev commented 24 June 2020 at 10:25

@daffie: no, I’m using 9.0 branch?

Comment #14

daffie commented 24 June 2020 at 10:27

@rik-dev: Did you do a git mv ...? See: https://git-scm.com/docs/git-mv.

Comment #15

daffie commented 24 June 2020 at 10:29

Status:

Needs review

» Needs work

@daffie: no, I’m using 9.0 branch?

The patch should land in the 9.1.x branch. Please use that branch.

Comment #16

rik-dev commented 24 June 2020 at 10:40

Status:

Needs work

» Needs review

Status	File	Size
new	3129560-5.patch	12.72 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	3129560-5.patch	12.67 KB

@daffie: sorry, Previously I made some patches for the 9.0 version and I didn't notice it. Thanks man:)

Comment #17

24 June 2020 at 11:38

Status:

Needs review

» Needs work

The last submitted patch, 16: 3129560-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Comment #18

rik-dev commented 24 June 2020 at 11:46

Status:

Needs work

» Needs review

Comment #19

daffie commented 24 June 2020 at 11:59

Waiting for the result of the testbot.

One nitpick:

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
@@ -6,6 +6,7 @@
+ *

This line does not need to be added here. The extra line in the docblock.

@rik-dev: Could you with the naming of patches take into account the comment number. Now they are all pointing to comment #5. Thank you for working on this issue.

Comment #20

rik-dev commented 24 June 2020 at 13:16

@daffie: Yeap, sure. Thanks for your feedback.

Comment #21

daffie commented 24 June 2020 at 13:18

Status:

Needs review

» Needs work

Back to needs work for the nitpick from comment #19.

Comment #22

rik-dev commented 24 June 2020 at 13:24

Status	File	Size
new	the-upsert-class-was-removed-for-PostgreSQL-database-driver-3129560-21.patch	12.58 KB

This is a little change from comment #19. I did it, no worries.

Comment #23

rik-dev commented 24 June 2020 at 21:43

Status:

Needs work

» Needs review

Comment #24

daffie commented 25 June 2020 at 07:07

Status:

Needs review

» Reviewed & tested by the community

1 file was hidden/shown/deleted

Status	File	Size
hidden	3129560-5.patch	12.72 KB

All code changes look good to me.
For me it is RTBC.

Comment #25

rik-dev commented 25 June 2020 at 09:35

Assigned:

rik-dev

» Unassigned

Comment #26

alexpott

he/they

English

🇪🇺🌍

commented 13 July 2020 at 22:07

Status:

Reviewed & tested by the community

» Needs work

Do we care about BC here? Like will this break the fallback driver. To me it looks like it will - see https://git.drupalcode.org/project/pgsql_fallback/-/blob/1.0.x/src/Drive...

Comment #27

daffie commented 14 July 2020 at 07:15

Status:

Needs work

» Reviewed & tested by the community

AFAIK there is no need for a BC layer. After this patch lands the PostgreSQL fallback driver has then only an unused class called NativeUpsert. Which can then safely be removed. Maybe I am missing something? If so, then please explain? For now, back to RTBC. Thank you @alexpott for thinking about a BC layer.

Comment #28

alexpott

he/they

English

🇪🇺🌍

commented 14 July 2020 at 08:18

Status:

Reviewed & tested by the community

» Fixed

  /**
   * {@inheritdoc}
   */
  public function minimumVersion() {
    return '9.6';
  }

Ah that's what I missed. From the fallback - so yeah there will be one unused class in the db driver. But that doesn't matter.

Committed 50a5a16 and pushed to 9.1.x. Thanks!

Comment #29

14 July 2020 at 08:19

alexpott committed 50a5a16 on 9.1.x

Issue #3129560 by rik-dev, daffie: Remove for PostgreSQL database driver...

Comment #30

amateescu commented 14 July 2020 at 09:06

Title:

Remove for PostgreSQL database driver the workaround for the Upsert class

» Remove the Upsert implementation for PostgreSQL < 9.5

Comment #31

amateescu commented 14 July 2020 at 09:07

Comment #32

28 July 2020 at 09:09

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Remove the Upsert implementation for PostgreSQL < 9.5

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Related issues

Referenced by