‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Sunday, May 19, 2019 2:58 PM, John Fawcett via dovecot <dovecot@dovecot.org> wrote:

I guess it is not supported for PostgreSQL or Sqlite, since they don't have "ON DUPLICATE KEY UPDATE" statement which is what is being used in MySQL.

That's it, PostgreSQL does not have "ON DUPLICATE KEY" but instead uses "ON CONFLICT" as documented here:

https://www.postgresql.org/docs/10/sql-insert.html#SQL-ON-CONFLICT

You could verify the query being used by turning on query logging in PostgresSQL.

I enabled query logging as suggested and found out that Dovecot dict is not using the "ON CONFLICT" feature of INSERT with PostgreSQL, as you can see from the query below:

INSERT INTO last_logins (last_login,username,domain) VALUES (1558273000,'user@domain.tld','domain.tld')

For me this makes Dovecot's dictionary feature useless with PostgreSQL. Should I open a bug for that? or is it more of a "feature request"?

Regards, Mabi

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Sunday, May 19, 2019 3:57 PM, Aki Tuomi <aki.tuomi@open-xchange.com> wrote:

It would be a feature request, also note that it's only available since version 9.5, so it really won't help anyone before that.

Would opening an issue as feature request on GitHub on the dovecot/core project be appropriate place for that?

I was thinking in the mean time as workaround that maybe with a trigger in PostgreSQL it should be possible to update the last_login column on the failing INSERT. Just need to find out how exactly to do that and I am not sure if it possible to execute a trigger when failing because of a duplicate key error.

On 19/05/2019 16:25, John Fawcett via dovecot wrote:

INSERT INTO last_logins (last_login,username,domain) VALUES (1558273000,'user@domain.tld','domain.tld')

I don't have PostgresSql, would you be able to verify if this syntax would work:

INSERT INTO last_logins (last_login,username,domain) VALUES (1558273000,'user@domain.tld','domain.tld') ONCONFLICT(username) UPDATE SET last_login=1558273000,domain='user@domain.tld'

It's important to check that this updates only the single row for that user and it puts the right data in that row. If it doesn't work can you give the correct syntax?

John

On 19/05/2019 17:08, mabi via dovecot wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Sunday, May 19, 2019 4:44 PM, John Fawcett via dovecot <dovecot@dovecot.org> wrote:

...

I don't have PostgresSql, would you be able to verify if this syntax would work: INSERT INTO last_logins (last_login,username,domain) VALUES (1558273000,'user@domain.tld <mailto:user@domain.tld>','domain.tld') ON CONFLICT(username) UPDATE SET last_login=1558273000,domain='user@domain.tld <mailto:user@domain.tld>' It's important to check that this updates only the single row for that user and it puts the right data in that row. If it doesn't work can you give the correct syntax?

So you nearly yes ;-) The only parameter missing was "DO" keyword before the "UPDATE". So the correct query would be:

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

I also adapted the domain='domain.tld' at the end of the query, you had domain='user@domain.tld <mailto:user@domain.tld>' but this is just a "content" detail which does not matter.

Hope that helps. Let me know if I can do any further testing.

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.

John

John

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@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

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@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@domain.tld <mailto:user@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

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@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

Maybe, one should drop the automatic at all and let the user specify the commands manually like with the userdb/passwd. Hence, the generic SQL preparation code is already present. There could/should/would be documented lots of "best practice" settings for various backends.

In fact, this approach would better fit into the open and more "general" base idea Dovecot uses in other places, IMHO.

Kind regards,

-- Steffen Kaiser

Hi!

You can write completely custom last_login plugin by using mail-lua plugin, by having functions

mail_user_created(user)

and

mail_user_deinit(user)

in your Lua script.

This of course requires v2.3.4 or later.

Aki

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Monday, May 20, 2019 12:37 AM, John Fawcett via dovecot <dovecot@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...

On May 29, 2019, at 2:01 PM, Aki Tuomi via dovecot <dovecot@dovecot.org> wrote:

We'll take this under consideration, but no promises.

Dear Aki,

I reported this issue and request before: https://marc.info/?t=155411531600001&r=1&w=2

Hope we can have this feature soon. :) Thank you very much.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Sunday, May 19, 2019 10:45 PM, John Fawcett via dovecot <dovecot@dovecot.org> wrote:

so basically if this works just as well:

INSERT INTO last_logins (last_login,username,domain) VALUES (1558273000,'user@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@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.