dovecot - May 2019 - more generic approach as for userdb? (was: Dict issue with PostgreSQL for last_login plugin (duplicate key))

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>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Wed, 22 May 2019, John Fawcett via dovecot wrote:
> 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.
You mean the "target" in ON CONFLICT target action, right?
http://www.postgresqltutorial.com/postgresql-upsert/
> 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).
One could dive into Postgres-specifics to get it, but there are other 
SQLs, too; the quota plugin advertises to use TRIGGERs to turn an INSERT 
into an UPDATE silently, which is no general approach either.
https://wiki2.dovecot.org/Quota/Dict
> 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.
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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEVAwUBXOY0bsQnQQNheMxiAQKNOQgAmRzNVJTNn3XpHBBGnZOtZ5Ku9Cp9UZIY
70HukeDKdR6rg7XNFGhwTDGa30QRGABByoospMHLAIabZ7j9WFaajAKI01roXotc
skD+T8orvpk7BH/2+f2v5f67xa3GU6LJE330yZJubFb87NFq4otdtXGjhPjCf16j
/wREiuSi0CqDTMtSOXjHXtViI9EL/e+CoJtEgK+gaXINCdCP7Cb2OEjtXHpItuqm
tUAQoh418wWfVt6k6NgpDVX/hD+RyRfxKI4dste0VJZ9OEhH1mpPGaRB/BIkhEh4
OJ18upVhIXbJPDyAPofSB1YGDkPl/HlChmh+QuOpVm9rolmt9SyZQg==unPo
-----END PGP SIGNATURE-----

> 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<https://dovecot.org/pipermail/dovecot/attachments/20190523/fe3de7c3/attachment-0001.sig>

On 23/05/2019 07:49, Steffen Kaiser via dovecot wrote:
> On Wed, 22 May 2019, John Fawcett via dovecot wrote:
>
> > 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.
>
> You mean the "target" in ON CONFLICT target action, right?
> http://www.postgresqltutorial.com/postgresql-upsert/
>
Yes, whereas MySQL uses a generic syntax not requiring specific info, as
far as I am aware PostgreSql requires the target. I tried without and
got an error.
> > 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).
>
> One could dive into Postgres-specifics to get it, but there are other
> SQLs, too; the quota plugin advertises to use TRIGGERs to turn an
> INSERT into an UPDATE silently, which is no general approach either.
> https://wiki2.dovecot.org/Quota/Dict
>
> > 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.
>
> 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.
>
thanks for that suggestion, it would mean moving away from a syntax
where other dictionary types use a map statement and sql
wouldn't.
> Kind regards,
>
> -- Steffen Kaiser