Mailing List Archive

>
> https://github.com/apache/spamassassin/blob/trunk/sql/awl_pg.sql#L6
>
> https://www.irccloud.com/pastebin/wRkT4AeI/awl.sql
>
> how to solve it ?

https://notepad.ltd/asdf23423asdfasdf ;)

Benny Pedersen wrote on 3/01/23 2:21 pm:
>
> https://github.com/apache/spamassassin/blob/trunk/sql/awl_pg.sql#L6
>
> https://www.irccloud.com/pastebin/wRkT4AeI/awl.sql
>
> how to solve it ?

The sql error means that there is more than one table in the sql
statement that has a column named "totscore" and so where the command
references "totscore" without using a syntax like tablename.totscore to
specify which table it means is ambiguous.

However, I don't see any definition of a table with a column named
totscore other than the one in table "awl" that you linked to. And the
full SQL query string is not in the snippet in your pastebin, that is
truncated

dbg: auto-welcomelist: [...] LINE 1: ...edby, ip) DO UPDATE set msgcount
= $6, totscore = totscore +...

If you can get the entire SQL command that is in that dbg output,
without truncation, it might be more obvious what is going on. Looking
at the source code in SQLBasedAddrList.pm, I don't see how the SQL query
that it generates can have any ambiguity as to which table totscore
refers to, there is just that one table as far as I can tell.

If you can't get the full dbg line, perhaps someone who actually uses
SQL based awl might be able to jump in here, that's the limit of what I
can figure out.

Sidney Markowitz skrev den 2023-01-03 10:53:
> Benny Pedersen wrote on 3/01/23 2:21 pm:
>>
>> https://github.com/apache/spamassassin/blob/trunk/sql/awl_pg.sql#L6
>>
>> https://www.irccloud.com/pastebin/wRkT4AeI/awl.sql
>>
>> how to solve it ?
>
> The sql error means that there is more than one table in the sql
> statement that has a column named "totscore" and so where the command
> references "totscore" without using a syntax like tablename.totscore
> to specify which table it means is ambiguous.
>
> However, I don't see any definition of a table with a column named
> totscore other than the one in table "awl" that you linked to. And the
> full SQL query string is not in the snippet in your pastebin, that is
> truncated
>
> dbg: auto-welcomelist: [...] LINE 1: ...edby, ip) DO UPDATE set
> msgcount = $6, totscore = totscore +...
>
> If you can get the entire SQL command that is in that dbg output,
> without truncation, it might be more obvious what is going on. Looking
> at the source code in SQLBasedAddrList.pm, I don't see how the SQL
> query that it generates can have any ambiguity as to which table
> totscore refers to, there is just that one table as far as I can tell.

https://github.com/apache/spamassassin/blob/trunk/lib/Mail/SpamAssassin/SQLBasedAddrList.pm#L310
imho this line

> If you can't get the full dbg line, perhaps someone who actually uses
> SQL based awl might be able to jump in here, that's the limit of what
> I can figure out.

it worked in 3.4.6

Benny Pedersen wrote on 4/01/23 3:19 am:
> https://github.com/apache/spamassassin/blob/trunk/lib/Mail/SpamAssassin/SQLBasedAddrList.pm#L310
> imho this line

I agree, but I don't see from looking at that line how the SQL query can
have more than one table involved, and from the description of what
"column reference is ambiguous" means I don't see how a query with one
table in it can get a column reference is ambiguous error.

>
>> If you can't get the full dbg line, perhaps someone who actually uses
>> SQL based awl might be able to jump in here, that's the limit of what
>> I can figure out.

I was wrong when I said that... The dbg line does not include the full
query anyway, just the arguments.

I don't know if the dbg line you get to is line 331 or 347, they both
look the same. However, if you can edit those to also output $sql you
can see the exact SQL query that is producing the error.

Someone who knows SQL better than I do could probably figure out from
that how it could produce a column reference is ambiguous error. Or if
you could add $sql to the dbg statements and post the full dbg output
without that truncation, I could probably Google enough to figure it out.

I'm not set up to easily test with postgres AWL here, so it would help
if you could generate the dbg lines with the additional info.

>
> it worked in 3.4.6

If anyone else reading this is using 4.0.0 and postgres for AWL, are you
seeing or not seeing this problem?

Sidney Markowitz skrev den 2023-01-03 22:24:
> Benny Pedersen wrote on 4/01/23 3:19 am:
>> https://github.com/apache/spamassassin/blob/trunk/lib/Mail/SpamAssassin/SQLBasedAddrList.pm#L310
>> imho this line
>
> I agree, but I don't see from looking at that line how the SQL query
> can have more than one table involved, and from the description of
> what "column reference is ambiguous" means I don't see how a query
> with one table in it can get a column reference is ambiguous error.

https://github.com/apache/spamassassin/blob/trunk/lib/Mail/SpamAssassin/SQLBasedAddrList.pm#L289

$sql .= " ON CONFLICT (username, email, signedby, ip) DO UPDATE set
msgcount = ?, totscore += ?";

confirm is from my side needed it would fix it, i atleast like to think
it does

there is more lines with totscore = totscore that might need fix aswell

On Wed, 2023-01-04 at 10:24 +1300, Sidney Markowitz wrote:
> Benny Pedersen wrote on 4/01/23 3:19 am:
>
> If anyone else reading this is using 4.0.0 and postgres for AWL, are
> you
> seeing or not seeing this problem?
>
I use Postgresql, though not with SA.

I agree with your suggestion, but it would also be useful to see the
definition of the table. 'psql', the postgres interactive command tool,
can be used to show this info: the psql command 

"\d tablename" 

displays columns in the 'tablename' table as well as other relevant
information about it. If Postgres was installed from a standard package,
the psql interactive program (and its manpage) should also have been
installed.

Martin

Martin Gregorie skrev den 2023-01-03 23:43:
> On Wed, 2023-01-04 at 10:24 +1300, Sidney Markowitz wrote:
>> Benny Pedersen wrote on 4/01/23 3:19 am:
>>
>> If anyone else reading this is using 4.0.0 and postgres for AWL, are
>> you
>> seeing or not seeing this problem?
>>
> I use Postgresql, though not with SA.
>
> I agree with your suggestion, but it would also be useful to see the
> definition of the table. 'psql', the postgres interactive command tool,
> can be used to show this info: the psql command 
>
> "\d tablename" 
>
> displays columns in the 'tablename' table as well as other relevant
> information about it. If Postgres was installed from a standard
> package,
> the psql interactive program (and its manpage) should also have been
> installed.

i have dumped all i have in posgres without data so only structure is
here

https://usercontent.irccloud-cdn.com/file/WJmDq7xc/spamassassin_dump_tables%20only.txt

dont know what package means on gentoo, its stable versions i use, just
not latest stable

if more info is needed i can provide it

On Wed, 2023-01-04 at 00:43 +0100, Benny Pedersen wrote:
>
> i have dumped all i have in posgres without data so only structure is
> here
>
> https://usercontent.irccloud-cdn.com/file/WJmDq7xc/spamassassin_dump_tables%20only.txt
>
> dont know what package means on gentoo, its stable versions i use,
> just
> not latest stable
>
> if more info is needed i can provide it

There's enough detail there to make an informed guess about what could
be wrong. 

The tables public.awl and public.txrep contain identical sets of column
names, so a reference to any of these column names will be rejected
unless it is qualified by referring to it as

table_name.column_name 

Without specifying the fully qualified name, e.g public.awl.email, the
database engine can't know which table contains the column that the
script its executing is meant to use.

Martin

On 2023-01-04 at 10:24 +1300, Sidney Markowitz wrote:
> If anyone else reading this is using 4.0.0 and postgres for AWL, are
> you seeing or not seeing this problem?

I can easily reproduce this with a quick install and manually providing
the SQL from the code (output included below). Postgresql (tested on
version 11) seem to be choking on the self-referential totscore in the
update clause.
It seems it can't assign a value from the row inside the DO UPDATE but
only a constant.

As a quick fix, removing the string “pg|” from the lines 309, 325, 341
and 358 will probably make the plugin work by making it use the
fallback code with postgres.


postgres=# CREATE TABLE awl (
postgres(# username varchar(100) NOT NULL default '',
postgres(# email varchar(255) NOT NULL default '',
postgres(# ip varchar(40) NOT NULL default '',
postgres(# msgcount bigint NOT NULL default '0',
postgres(# totscore float NOT NULL default '0',
postgres(# signedby varchar(255) NOT NULL default '',
postgres(# last_hit timestamp NOT NULL default CURRENT_TIMESTAMP,
postgres(# PRIMARY KEY (username,email,signedby,ip)
postgres(# );
CREATE TABLE
postgres=# select * from awl;
username | email | ip | msgcount | totscore | signedby | last_hit
----------+-------+----+----------+----------+----------+----------
(0 rows)

postgres=# insert into awl (username, email, ip, signedby) values ('john', 'jsmith@example.com', '127.0.0.1', '-');
INSERT 0 1
postgres=# select * from awl;
username | email | ip | msgcount | totscore | signedby | last_hit
----------+--------------------+-----------+----------+----------+----------+----------------------------
john | jsmith@example.com | 127.0.0.1 | 0 | 0 | - | 2023-01-04 02:45:24.769152
(1 row)

postgres=# insert into awl (username, email, ip, signedby) values ('john', 'jsmith@example.com', '127.0.0.1', '-') ON CONFLICT (username, email, signedby, ip) DO UPDATE set msgcount = 5, totscore=totscore + 10;
ERROR: column reference "totscore" is ambiguous
LINE 1: ...ignedby, ip) DO UPDATE set msgcount = 5, totscore=totscore +...
^

On 2023-01-03 at 23:00 +0100, Benny Pedersen wrote:
> https://github.com/apache/spamassassin/blob/trunk/lib/Mail/SpamAssassin/SQLBasedAddrList.pm#L289
>
> $sql .= " ON CONFLICT (username, email, signedby, ip) DO UPDATE set
> msgcount = ?, totscore += ?";
>
> confirm is from my side needed it would fix it, i atleast like to
> think it does

A += doesn't (neccesarily) fix it. Perhaps it works on an higher
version.

postgres=# insert into awl (username, email, ip, signedby) values ('john', 'jsmith@example.com', '127.0.0.1', '-') ON CONFLICT (username, email, signedby, ip) DO UPDATE set msgcount = 5, totscore += 1;
ERROR: syntax error at or near "+="
LINE 1: ...il, signedby, ip) DO UPDATE set msgcount = 5, totscore += 1;
^

Ángel wrote on 4/01/23 2:59 pm:
> On 2023-01-04 at 10:24 +1300, Sidney Markowitz wrote:
>> If anyone else reading this is using 4.0.0 and postgres for AWL, are
>> you seeing or not seeing this problem?
>
> I can easily reproduce this with a quick install and manually providing
> the SQL from the code (output included below). Postgresql (tested on
> version 11) seem to be choking on the self-referential totscore in the
> update clause.
> It seems it can't assign a value from the row inside the DO UPDATE but
> only a constant.

Thanks, I found it easy to reproduce your results by pasting that into
the online sandbox at https://www.db-fiddle.com/ and running with every
version of postgres that has ON CONFLICT (added in version 9.5).

Martin, that proves that it doesn't have anything to do with the awl and
txrep tables having the same column names, it is something postgres is
picky about even with no other tables in the schema. SQLite, which uses
the same ON CONFLICT syntax for upsert doesn't sem to have a problem
with it.
>
> As a quick fix, removing the string “pg|” from the lines 309, 325, 341
> and 358 will probably make the plugin work by making it use the
> fallback code with postgres.

> A += doesn't (neccesarily) fix it. Perhaps it works on an higher
> version.

The correct syntax is =+ Benny's += was probably a typo.

I verified in the sandbox that it works for every version of postgres
from 9.5 through 15, and also the SQLite versions there.

Making the expression totscore = awl.totscore + 10 also works, but =+
seems cleaner.

I'll open a bug report, post a patch that gets all of the similar sql
statements, and see what the devs with more SQL experience have to say.

-- Sidney

Benny Pedersen wrote on 4/01/23 11:00 am:
> https://github.com/apache/spamassassin/blob/trunk/lib/Mail/SpamAssassin/SQLBasedAddrList.pm#L289
>
> $sql .= " ON CONFLICT (username, email, signedby, ip) DO UPDATE set
> msgcount = ?, totscore += ?";
>
> confirm is from my side needed it would fix it, i atleast like to think
> it does

There's a typo, which must just be in your email since postgres won't
accept it, that should be =+ not +=

>
> there is more lines with totscore = totscore that might need fix aswell
>

I checked that, and the only two other places I saw were conditional on
not being postgres. Also, the last place it is used, in the fallback
UPDATE statement, I tried it in postgres and found that in that
statement postgres allows the totscore = totscore + 10 syntax. Strange.

I opened a bug https://bz.apache.org/SpamAssassin/show_bug.cgi?id=8098
and will submit a patch for that =+ change.

Thanks for catching this.

Sidney

Sidney Markowitz wrote on 4/01/23 8:47 pm:
>
> There's a typo, which must just be in your email since postgres won't
> accept it, that should be =+ not +=

I am not expert it SQL :)

Further testing reveals that there is no auto-increment operator in
postgres or SQLite SQL, neither += nor =+

The reason why =+ does not get a syntax error is because it is parsed as
two separate operators and whitespace is not required and is ignored. In
other words, totspace =+ 10 is the same as totspace = +10 which sets
totspace to the value 10, which is not what was intended.

The change to =+ doesn't fail the syntax, but does cause test
t/sql_based_welcomelist.t to fail because the results are wrong.

The only syntax that actually works is to include the table name as in

totspace = awl.totspace + 10

I have submitted a patch to that effect in bug 8098 for people who do
know SQL to review.

Benny, if you want to try out that patch, please do so.

Sidney