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

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 Monday, May 20, 2019 12:37 AM, John Fawcett via dovecot <dovecot at
dovecot.org> wrote:
> 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).
So as you mention the new query you adapted which includes the primary key
works, I tested it manually against PostgreSQL 10.5.
> The attached fix should work in that case, although I feel it's not
general enough.
Unfortunately my compiling skills are quite poor and I did not manage to patch
and recompile Dovecot on OpenBSD.

Do you think your patch will make it into the Dovecot code?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://dovecot.org/pipermail/dovecot/attachments/20190521/00f716a1/attachment.html>

On 21/05/2019 15:45, mabi via dovecot wrote:
> ??????? Original Message ???????
> On Monday, May 20, 2019 12:37 AM, John Fawcett via dovecot
> <dovecot at dovecot.org> wrote:
>
>> 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).
>>
> So as you mention the new query you adapted which includes the primary
> key works, I tested it manually against PostgreSQL 10.5.
>>
>> The attached fix should work in that case, although I feel it's not
>> general enough.
>>
> Unfortunately my compiling skills are quite poor and I did not manage
> to patch and recompile Dovecot on OpenBSD.
>
> Do you think your patch will make it into the Dovecot code?
>
I feel confident that the patch works as the query has been manually
verified and the code change is not complex to validate.

The last_login plugin does not work at the moment with PostgreSql and
probably does not work with Sqlite, given that the only logic that tries
an update when insert fails seems to be a MySQL specific extension to
standard Sql. So I think that it's clear that support for PostgreSql and
Sqlite? needs to be implemented. The same issue likely exist in other
plugins too, for example expire.

My doubts are around the right solution to adopt. Initially I thought
that there was a PostgreSql syntax similar to MySQL which could be
easily added to the code, but closer inspection shows that the
PostgreSql syntax requires specification of either a constraint name or
the index column(s) for the primary/unique keys.

Constraint names are nowhere specified in the dictionary map syntax and
it's not possible either to identify with 100% certainty the primary key
column(s).

The solution I adopted in the latest version of the patch is to use the
default primary key constraint name derived from the table name, but
that won't help if people define custom constraint names. That may be an
unlikely scenario so the fix is certainly better than AS-IS. However it
is not perfect and added to that is the fact that the PostgreSql
extension is available only from 9.5.

I have no issues to submit the patch officially, as long as Dovecot
developers agree. However it may be worthwhile reflecting on a more
structural change

1) logic which always tries to update and falls back to insert if the
update fails (or viceversa) for all sql dictionaries.

2) updates to the map syntax so that either the constraint name or
primary key columns can be specified.

Ideas are welcome.

John


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

??????? Original Message ???????
On Monday, May 20, 2019 12:37 AM, John Fawcett via dovecot <dovecot at
dovecot.org> wrote:
> 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.
I saw there has been quite some discussion how to make things more generic and
better for database queries in general in Dovecot around my issue but I would
still be very thankful if your original patch could be submitted to Dovecot for
review and approval. Your patch solves an immediate problem which is of adding
UPSERT functionality to PostgreSQL Dict queries.

MySQL Dict queries has its "INSERT ... ON DUPLICATE KEY UPDATE"
implemented in Dovecot so I think it's more than fair that for now that
PostgreSQL support in Dict also gets its equivalent "INSERT ... ON CONFLICT
UPDATE" implemented.

This is just my opinion as a long-time "user" of Dovecot, I am no
dev...
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://dovecot.org/pipermail/dovecot/attachments/20190528/ca7aabc5/attachment.html>

On 28.5.2019 22.34, mabi via dovecot wrote:
> ??????? Original Message ???????
> On Monday, May 20, 2019 12:37 AM, John Fawcett via dovecot
> <dovecot at dovecot.org> wrote:
>
>> 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.
>>
> I saw there has been quite some discussion how to make things more
> generic and better for database queries in general in Dovecot around
> my issue but I would still be very thankful if your original patch
> could be submitted to Dovecot for review and approval. Your patch
> solves an immediate problem which is of adding UPSERT functionality to
> PostgreSQL Dict queries.
>
> MySQL Dict queries has its "INSERT ... ON DUPLICATE KEY UPDATE"
> implemented in Dovecot so I think it's more than fair that for now
> that PostgreSQL support in Dict also gets its equivalent "INSERT ...
> ON CONFLICT UPDATE" implemented.
>
> This is just my opinion as a long-time "user" of Dovecot, I am no
dev...
We'll take this under consideration, but no promises.

Aki

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