Issue 12729 postgresql duplicate key value

[oole]

This should fix the

ERROR: duplicate key value violates unique constraint "lock_key_index"

errors filling up the PostgreSQL log. As described by Issue 12729.

I moved the insert statement from the DBLockingProvider.php to the Adapter.php, so that database specific inserts are possible.

[MorrisJobke]

Code wise makes a lot of sense. 👍 Let me test this later and then I'm fine with getting this in.

[kesselb]

Please add insertIgnoreConflict to IDBConnection. In line 121 is the definition for the now depcreated insertIfNotExists if you need an example.

[MorrisJobke]

Please add insertIgnoreConflict to IDBConnection. In line 121 is the definition for the now depcreated insertIfNotExists if you need an example.

We need to be careful here as it is then a public API we need to keep stable. Maybe we add some PHPDoc to indicate that this is not fully finished and should not be used for now? Maybe we find some cases in the near future where we need to change the API a little bit.

[kesselb]

Please add insertIgnoreConflict to IDBConnection. In line 121 is the definition for the now depcreated insertIfNotExists if you need an example.

We need to be careful here as it is then a public API we need to keep stable. Maybe we add some PHPDoc to indicate that this is not fully finished and should not be used for now? Maybe we find some cases in the near future where we need to change the API a little bit.

Is not adding the method to the interface an option? Technically it works 🤔

[icewind1991]

I would prefer to add the "on conflict" handing to the query builder

[kesselb]

I would prefer to add the "on conflict" handing to the query builder

Then query builder has to know which adapter is used by the current connection. Would you prefer to a) introduce child classes QueryBuilderPostgres extends QueryBuilder or b) Pass the type of adapter to QueryBuilder (like below for Connection)

	public function getQueryBuilder() {
		return new QueryBuilder(
			$this,
			\OC::$server->getSystemConfig(),
			\OC::$server->getLogger(),
			get_class($this->adapter)
		);
	}

I guess both ways have their pros/cons. Maybe you come up with a better idea.

[icewind1991]

You can already determine the sql backend used within the query builder, see the expr and func methods.

but since this probably makes most sense in the QueryBuilder itself and not a "sub builder" subclassing the QueryBuilder is probably best

[oole]

I finally found time to get back to this, thanks for staying with me.

I applied the proposed changes to the existing approach, meaning

• Some cleanup
• The addition of the function to the IDBConnection interface
• Addition of the SqlInjectionChecker

Though I am interested in the approach of adding it to the QueryBuilder. How would that work, would I just add a function 'upsert' or 'updateIgnoreConflict' to the QueryBuilder that builds the proposed query, and calls the Adapter to build/add the database-specific part to the query? The this could be used directly in the DBLockingProvider.

[MorrisJobke]

With which version of Postgres does this work? I only can find it in the docs since Postgres 9.5.

[blizzz]

Aye. Caused this regression: #15613

[MorrisJobke]

😢

[MorrisJobke]

Code looks good so far.

@icewind1991 Mind to test this?

[MorrisJobke]

I'm fine with this change 👍

[MorrisJobke]

@oole Do you mind to rebase on the latest master because we fixed some failing tests there.

[oole]

Happy to read! Of course, rebased and pushed.

[blizzz]

What about #13721 (comment) ? Afaik, 9.4 is still suppported until end of this year.

[MorrisJobke]

$ ./lib/composer/phan/phan/phan -k build/.phan/config.php
lib/private/DB/Adapter.php:138 SqlInjectionChecker Potential SQL injection detected - method: no child method
lib/private/DB/AdapterPgSql.php:46 SqlInjectionChecker Potential SQL injection detected - method: no child method

I pushed a fix for this. Was a missing *.

[MorrisJobke]

@kesselb @rullzer @ChristophWurst Mind to review? I would like to get this into beta 1 to have as much test coverage as possible.

[rullzer]

Lets do this. And test the hell out of it in real life as well 🚀 🐘

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!