dovecot - May 2019 - Dict issue with PostgreSQL for last_login plugin (duplicate key)

On 19/05/2019 20:31, mabi via dovecot wrote:
>
> ??????? Original Message ???????
> On Sunday, May 19, 2019 7:36 PM, John Fawcett via dovecot
> <dovecot at dovecot.org> wrote:
>>
>> Attached is a tentative patch. I've verified no regression for
mysql.
>> There should be no regression for sqlite as the code path is identical.
>>
>> Are you able to test for pgsql? As mentioned by Akie it will break
>> for PostgresSql < 9.5 but probably it was not working anyway due to
>> duplicate keys. Whether this is a wider problem depends on whether
>> the insert code is being used for other purposes too.
>>
>> If you or someone can verify it works on PostgresSql >= 9.5, then
the
>> next step will be to make it conditional on the version.
>>
> Thank you very much John for your patch, that's fantastic. I am on
> OpenBSD 6.5 and will recompile dovecot from the ports by adding your
> patch to it, I hope that works and will let you know if I managed. If
> I understand correctly the relevant binary file I need to replace is
> the following right:
>
> /usr/local/lib/dovecot/dict/libdriver_pgsql.so
>
> or are there any others I also need to replace in order to test? I am
> planning to test live by just replacing the relevant file(s) so that I
> hopefully don't need to re-install the whole dovecot package.
I'm not sure how the source compilation works on OpenBSD, when I do it
on linux and run "make install" it installs all relevant
binaries/libraries.

I saw one issue with the fix though, it does not correctly pull out the
username field. I'm wondering if the query can be rewritten not to
mention the name of the field that fails the constraint....

John

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://dovecot.org/pipermail/dovecot/attachments/20190519/f66b6144/attachment.html>

On 19/05/2019 22:37, John Fawcett via dovecot wrote:
> On 19/05/2019 20:31, mabi via dovecot wrote:
>>
>> ??????? Original Message ???????
>> On Sunday, May 19, 2019 7:36 PM, John Fawcett via dovecot
>> <dovecot at dovecot.org> wrote:
>>>
>>> Attached is a tentative patch. I've verified no regression for
>>> mysql. There should be no regression for sqlite as the code path is
>>> identical.
>>>
>>> Are you able to test for pgsql? As mentioned by Akie it will break
>>> for PostgresSql < 9.5 but probably it was not working anyway due
to
>>> duplicate keys. Whether this is a wider problem depends on whether
>>> the insert code is being used for other purposes too.
>>>
>>> If you or someone can verify it works on PostgresSql >= 9.5,
then
>>> the next step will be to make it conditional on the version.
>>>
>> Thank you very much John for your patch, that's fantastic. I am on
>> OpenBSD 6.5 and will recompile dovecot from the ports by adding your
>> patch to it, I hope that works and will let you know if I managed. If
>> I understand correctly the relevant binary file I need to replace is
>> the following right:
>>
>> /usr/local/lib/dovecot/dict/libdriver_pgsql.so
>>
>> or are there any others I also need to replace in order to test? I am
>> planning to test live by just replacing the relevant file(s) so that
>> I hopefully don't need to re-install the whole dovecot package.
>
> I'm not sure how the source compilation works on OpenBSD, when I do it
> on linux and run "make install" it installs all relevant
> binaries/libraries.
>
> I saw one issue with the fix though, it does not correctly pull out
> the username field. I'm wondering if the query can be rewritten not to
> mention the name of the field that fails the constraint....
>
> John
>
so basically if this works just as well:

INSERT INTO last_logins (last_login,username,domain) VALUES
(1558273000,'user at domain.tld <mailto:user at
domain.tld>','domain.tld') ON
CONFLICT DO UPDATE SET last_login=1558273000,domain='domain.tld';

then the fix can be altered to attached file which is more similar to
the MYSQL syntax and does not require extra logic to get the username field.

John



-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://dovecot.org/pipermail/dovecot/attachments/20190519/b0ab52cc/attachment.html>
-------------- next part --------------
--- dict-sql-private.h.orig	2019-05-19 19:00:12.395887496 +0200
+++ dict-sql-private.h	2019-05-19 19:04:00.147601310 +0200
@@ -13,6 +13,7 @@
 	HASH_TABLE(const char *, struct sql_prepared_statement *) prep_stmt_hash;
 
 	bool has_on_duplicate_key:1;
+	bool has_on_conflict_do_update:1;
 };
 
 #endif
--- dict-sql.c.orig	2019-05-19 18:58:02.435194691 +0200
+++ dict-sql.c	2019-05-19 19:17:52.613253822 +0200
@@ -105,8 +105,10 @@
 	i_zero(&sql_set);
 	sql_set.driver = driver->name;
 	sql_set.connect_string = dict->set->connect;
-	/* currently pgsql and sqlite don't support "ON DUPLICATE KEY"
*/
+	/* pgsql and sqlite don't support "ON DUPLICATE KEY" */
+	/* mysql and sqlite don't support "ON CONFLICT DO UPDATE" */
 	dict->has_on_duplicate_key = strcmp(driver->name, "mysql") ==
0;
+	dict->has_on_conflict_do_update = strcmp(driver->name,
"pgsql") == 0;
 
 	if (sql_db_cache_new(dict_sql_db_cache, &sql_set, &dict->db,
error_r) < 0) {
 		pool_unref(&pool);
@@ -1108,12 +1110,15 @@
 
 	str_append_str(prefix, suffix);
 	str_append_c(prefix, ')');
-	if (!dict->has_on_duplicate_key) {
+	if (dict->has_on_duplicate_key ) {
+		str_append(prefix, " ON DUPLICATE KEY UPDATE ");
+	} else if(dict->has_on_conflict_do_update)  {
+		str_append(prefix, " ON CONFLICT DO UPDATE SET ");
+	} else {
 		*stmt_r = sql_dict_transaction_stmt_init(ctx, str_c(prefix), &params);
 		return 0;
 	}
 
-	str_append(prefix, " ON DUPLICATE KEY UPDATE ");
 	for (i = 0; i < field_count; i++) {
 		const char *first_value_field  			t_strcut(fields[i].map->value_field,
',');

On 19/05/2019 22:45, John Fawcett via dovecot wrote:
> On 19/05/2019 22:37, John Fawcett via dovecot wrote:
>> On 19/05/2019 20:31, mabi via dovecot wrote:
>>>
>>> ??????? Original Message ???????
>>> On Sunday, May 19, 2019 7:36 PM, John Fawcett via dovecot
>>> <dovecot at dovecot.org> wrote:
>>>>
>>>> Attached is a tentative patch. I've verified no regression
for
>>>> mysql. There should be no regression for sqlite as the code
path is
>>>> identical.
>>>>
>>>> Are you able to test for pgsql? As mentioned by Akie it will
break
>>>> for PostgresSql < 9.5 but probably it was not working anyway
due to
>>>> duplicate keys. Whether this is a wider problem depends on
whether
>>>> the insert code is being used for other purposes too.
>>>>
>>>> If you or someone can verify it works on PostgresSql >= 9.5,
then
>>>> the next step will be to make it conditional on the version.
>>>>
>>> Thank you very much John for your patch, that's fantastic. I am
on
>>> OpenBSD 6.5 and will recompile dovecot from the ports by adding
your
>>> patch to it, I hope that works and will let you know if I managed.
>>> If I understand correctly the relevant binary file I need to
replace
>>> is the following right:
>>>
>>> /usr/local/lib/dovecot/dict/libdriver_pgsql.so
>>>
>>> or are there any others I also need to replace in order to test? I
>>> am planning to test live by just replacing the relevant file(s) so
>>> that I hopefully don't need to re-install the whole dovecot
package.
>>
>> I'm not sure how the source compilation works on OpenBSD, when I do
>> it on linux and run "make install" it installs all relevant
>> binaries/libraries.
>>
>> I saw one issue with the fix though, it does not correctly pull out
>> the username field. I'm wondering if the query can be rewritten not
>> to mention the name of the field that fails the constraint....
>>
>> John
>>
> so basically if this works just as well:
>
> INSERT INTO last_logins (last_login,username,domain) VALUES
> (1558273000,'user at domain.tld <mailto:user at
domain.tld>','domain.tld')
> ON CONFLICT DO UPDATE SET
last_login=1558273000,domain='domain.tld';
>
> then the fix can be altered to attached file which is more similar to
> the MYSQL syntax and does not require extra logic to get the username
> field.
>
> John
>
>
So looking into this with a postgresql databse to work with: the above
query does not work. You have to specify either the column name or the
constraint name that you expect to be violated in order for the update
to take place.

With a map like this one you're using

|map { pattern = shared/last-login/$user/$domain table = last_login
value_field = last_login value_type = uint fields { username = $user
domain = $domain } }|

there's no field name that is obviously the primary key. I've reworked
the patch to use the postgres default primary key constraint name
(tablename_pkey).

The attached fix should work in that case, although I feel it's not
general enough.

John

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://dovecot.org/pipermail/dovecot/attachments/20190520/6d0f6850/attachment-0001.html>
-------------- next part --------------
--- dict-sql-private.h.orig	2019-05-19 19:00:12.395887496 +0200
+++ dict-sql-private.h	2019-05-19 19:04:00.147601310 +0200
@@ -13,6 +13,7 @@
 	HASH_TABLE(const char *, struct sql_prepared_statement *) prep_stmt_hash;
 
 	bool has_on_duplicate_key:1;
+	bool has_on_conflict_do_update:1;
 };
 
 #endif
--- dict-sql.c.orig	2019-05-19 18:58:02.435194691 +0200
+++ dict-sql.c	2019-05-20 00:25:56.558251260 +0200
@@ -105,8 +105,10 @@
 	i_zero(&sql_set);
 	sql_set.driver = driver->name;
 	sql_set.connect_string = dict->set->connect;
-	/* currently pgsql and sqlite don't support "ON DUPLICATE KEY"
*/
+	/* pgsql and sqlite don't support "ON DUPLICATE KEY" */
+	/* mysql and sqlite don't support "ON CONFLICT DO UPDATE" */
 	dict->has_on_duplicate_key = strcmp(driver->name, "mysql") ==
0;
+	dict->has_on_conflict_do_update = strcmp(driver->name,
"pgsql") == 0;
 
 	if (sql_db_cache_new(dict_sql_db_cache, &sql_set, &dict->db,
error_r) < 0) {
 		pool_unref(&pool);
@@ -1108,12 +1110,15 @@
 
 	str_append_str(prefix, suffix);
 	str_append_c(prefix, ')');
-	if (!dict->has_on_duplicate_key) {
+	if (dict->has_on_duplicate_key ) {
+		str_append(prefix, " ON DUPLICATE KEY UPDATE ");
+	} else if(dict->has_on_conflict_do_update)  {
+		str_printfa(prefix, " ON CONFLICT ON CONSTRAINT %s_pkey DO UPDATE SET
",fields[0].map->table);
+	} else {
 		*stmt_r = sql_dict_transaction_stmt_init(ctx, str_c(prefix), &params);
 		return 0;
 	}
 
-	str_append(prefix, " ON DUPLICATE KEY UPDATE ");
 	for (i = 0; i < field_count; i++) {
 		const char *first_value_field  			t_strcut(fields[i].map->value_field,
',');

??????? Original Message ???????
On Sunday, May 19, 2019 10:45 PM, John Fawcett via dovecot <dovecot at
dovecot.org> wrote:
> so basically if this works just as well:
>
> INSERT INTO last_logins (last_login,username,domain) VALUES
(1558273000,'user at domain.tld','domain.tld') ON CONFLICT DO
UPDATE SET last_login=1558273000,domain='domain.tld';
>
> then the fix can be altered to attached file which is more similar to the
MYSQL syntax and does not require extra logic to get the username field.
Unfortunately this query does not work, it looks like it really requires the
column name as you can see below from the error message:

ERROR:  ON CONFLICT DO UPDATE requires inference specification or constraint
name
LINE 1: ...ain) VALUES (1558273000,'user at
domain.tld','domain.tld') ON CONFLIC...
                                                             ^
HINT:  For example, ON CONFLICT (column_name).

But if you can use the table's primary key as default for the column name as
you mention in your other mail then that should work. I am using here PostgreSQL
10.5 by the way.

I still haven't figured out yet how to recompile properly Dovecot on OpenBSD
with your patch but I will give it another shot tonight.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://dovecot.org/pipermail/dovecot/attachments/20190520/6cc22694/attachment.html>