Mentat

Based on discussion, the empty array case can be detected and enforced in the DB as follows:

ALTER TABLE reports_events ADD CONSTRAINT mail_to_non_empty_array CHECK (mail_to <> '{}');

The first question is - it this information actually accessed/used somewhere after write?

In relation to #7257, do I understand it right that this column bears third type of information - what emails the report actually went to, and that this may diverge from what was in the mail headers?

Pavel Kácha wrote in #note-6:

The first question is - it this information actually accessed/used somewhere after write?

Yes. The values of this this attribute/array are shown in the detail of a report, in the tab 'Metadata', row 'Target email'.

In relation to #7257, do I understand it right that this column bears third type of information - what emails the report actually went to, and that this may diverge from what was in the mail headers?

Correct, it stores actual email addresses that the report went to. So for example, on mentat-alt there is always 'root'. Mail headers change either because of the configuration setting or because the abuse group has option 'report redirection' enabled.

So - correct solution after discussion would be:

• we should not have two values representing one state, so let's settle on empty array for generated but unsent reports and migrate column to not null (and update null -> [])
• repair known wrong values (strip leading/trailing whitespace, 'none', or maybe check nonconforming emails), where we are pretty sure we won't break good data

Of course, depends on what worms are going to crawl from the can, we can stil reconsider.

Added #7215 as blocker, as it's the culprit of the wrong data here. We have to solve that first to prevent new wrong data creeping in here.

A review was requested, here are my remarks (in no particular order):

1. I would use FOREACH for the loop, as follows:
   

      FOREACH element IN ARRAY arr LOOP
       array_append(retVal, trim(element));
      END LOOP;
   

   
   Of course, there is just a syntactic difference. Both do exactly the same thing, so pick by personal taste. (Warning: I did not run that code).
2. The case of mail_to = '{None}' represents a situation when the report was generated and was intended to be sent, but that failed (as it happens, always due to the message being too big, but possibly due to other causes). I am against flatly deleting such unsuccessfully sent reports. Setting them to empty array (as the NULL ones) further hides the distinction (from those not intended to be sent), but that should be marked in logs if the case occurred recently and I guess is of diminishing usefulness over time. If it is important, then this might need an additional attribute or logic (I am unsure whether the mail_res is displayed in the UI somewhere).
3. The created function is left lingering. I would add something like this somewhere towards the end:
   

   DROP FUNCTION trim_array(character varying[]);
   

4. The code as it stands alters all rows of the table. There is little that can be done to prevent this. Some sort of {VACUUM FULL, CLUSTER, pg_repack} should be performed at the end, otherwise the table and especially the indices would remain horribly bloated. The table is small (<750MB on production), so it would take a short time. Of course, this could be just documented and left for the DB admin.
5. I would advise on prefixing the names of functions used only during migrations (or any other such temporary entities) with some obvious identifier ('alembic_' + alembic migration id?). It is a rare case (this is only the second time we needed any sort of custom function for migration), but that lowers the chance of a naming conflict and better shows the purpose if a forgotten entity is later found in the DB.

I should be available this week for any follow-up.

Thank you so much for your review. The updated version can be found at the end of this note.

Radko Krkoš wrote in #note-14:

A review was requested, here are my remarks (in no particular order):

1. I would use FOREACH for the loop, as follows:
   [...]
   Of course, there is just a syntactic difference. Both do exactly the same thing, so pick by personal taste. (Warning: I did not run that code).

I tried running the function with this change and I had to change it a bit. array_append returns an array, so I had to reassign the variable. Also, the trim function is returning type text, so I had to cast it to varchar.

1. The case of mail_to = '{None}' represents a situation when the report was generated and was intended to be sent, but that failed (as it happens, always due to the message being too big, but possibly due to other causes). I am against flatly deleting such unsuccessfully sent reports. Setting them to empty array (as the NULL ones) further hides the distinction (from those not intended to be sent), but that should be marked in logs if the case occurred recently and I guess is of diminishing usefulness over time. If it is important, then this might need an additional attribute or logic (I am unsure whether the mail_res is displayed in the UI somewhere).

Instead of deleting those reports, I changed the mail_to to empty array. Those reports are from 2017, so I don't think they are useful anymore.

1. The created function is left lingering. I would add something like this somewhere towards the end:
   [...]
2. I would advise on prefixing the names of functions used only during migrations (or any other such temporary entities) with some obvious identifier ('alembic_' + alembic migration id?). It is a rare case (this is only the second time we needed any sort of custom function for migration), but that lowers the chance of a naming conflict and better shows the purpose if a forgotten entity is later found in the DB.

I renamed the function and added the drop statement at the end.

1. The code as it stands alters all rows of the table. There is little that can be done to prevent this. Some sort of {VACUUM FULL, CLUSTER, pg_repack} should be performed at the end, otherwise the table and especially the indices would remain horribly bloated. The table is small (<750MB on production), so it would take a short time. Of course, this could be just documented and left for the DB admin.

I tried running CLUSTER VERBOSE reports_events on my local machine with 1010 reports and it seems like not much has happened. I will add it to the migration though (without the verbose option). Is it also important in downgrade? The downgrade will only remove the NOT NULL constraint.

Here is the updated version:

CREATE OR REPLACE FUNCTION alembic_4a172cd00ef0_trim_array(character varying[])
RETURNS character varying[]
AS
$$
DECLARE
   arr ALIAS FOR $1;
   element character varying;
   retVal character varying[];
BEGIN
   FOREACH element IN ARRAY arr
   LOOP
    retVal := array_append(retVal, trim(element)::varchar);
   END LOOP;
RETURN retVal;
END;    
$$
LANGUAGE plpgsql 
   STABLE 
RETURNS NULL ON NULL INPUT;

update reports_events set mail_to = ARRAY[]::varchar[] where mail_to is null or mail_to = '{None}';
update reports_events set mail_to = alembic_4a172cd00ef0_trim_array(mail_to);
alter table reports_events alter column mail_to set not null;
DROP FUNCTION alembic_4a172cd00ef0_trim_array(character varying[]);
CLUSTER reports_events;

It is also worth mentioning that because this task will be merged before #6227 which has some migrations, Alembic merge will be needed at some point.

Rajmund Hruska wrote in #note-15:

Thank you so much for your review. The updated version can be found at the end of this note.

No problem.

Instead of deleting those reports, I changed the mail_to to empty array. Those reports are from 2017, so I don't think they are useful anymore.

Yes, they are probably not, but we keep full history there yet and punching holes is against that.

I tried running CLUSTER VERBOSE reports_events on my local machine with 1010 reports and it seems like not much has happened.

The whole table and its indices was rewritten from scratch. On such a small table, it is a quick operation. There are definitely differences in table and index sizes, albeit small. It is also much more efficient to do any operation on the table.

I will add it to the migration though (without the verbose option).

We did not do that before. This should better be discussed before taking action.

Is it also important in downgrade? The downgrade will only remove the NOT NULL constraint.

No, as no modification to table data is done in that case, only a change in table metadata. The removal of NOT NULL constraint is trivial, does not even need a scan (the data is logically guaranteed to be a subset of the new 'constraints').

Here is the updated version:
[...]

Looks good to me.

It is also worth mentioning that because this task will be merged before #6227 which has some migrations, Alembic merge will be needed at some point.

Well, if I am not missing something, the migration from #6227 can still be rebased on this one by changing the down_revision attribute (plus the comments). It is not like that one was deployed anywhere. OK, except for your development environment, but that should be fixable locally and not blocking.
Even this one might need such a 'rebase' as it is blocked by #7215, which might need a migration of its own.

Instead of deleting those reports, I changed the mail_to to empty array. Those reports are from 2017, so I don't think they are useful anymore.

Is the mail_res displayed somewhere in the report view? If not, maybe it should be added.

Radko Krkoš wrote in #note-17:

Instead of deleting those reports, I changed the mail_to to empty array. Those reports are from 2017, so I don't think they are useful anymore.

Is the mail_res displayed somewhere in the report view? If not, maybe it should be added.

mail_res contains basically the same information as mail_to. Moreover, the reports in the last 3 years have this column empty. Even in the current code this attribute is never assigned nor used, so it seems like it was deleted from the code but not from the database.

Rajmund Hruska wrote in #note-18:

Radko Krkoš wrote in #note-17:

Is the mail_res displayed somewhere in the report view? If not, maybe it should be added.

mail_res contains basically the same information as mail_to. Moreover, the reports in the last 3 years have this column empty. Even in the current code this attribute is never assigned nor used, so it seems like it was deleted from the code but not from the database.

OK, then I retract that.

I made the Alembic migration in 096d69c4. As adding CLUSTER reports_events into the migration file is still to be discussed, I will keep the 'To be discussed' flag set to 'Yes'.

Radko Krkoš wrote in #note-16:

Rajmund Hruska wrote in #note-15:

I tried running CLUSTER VERBOSE reports_events on my local machine with 1010 reports and it seems like not much has happened.

The whole table and its indices was rewritten from scratch. On such a small table, it is a quick operation. There are definitely differences in table and index sizes, albeit small. It is also much more efficient to do any operation on the table.

I will add it to the migration though (without the verbose option).

We did not do that before. This should better be discussed before taking action.

I think we have already discussed this and settled against adding VACUUM/CLUSTER/ANALYZE into migrations (see #7215), also #7538 started from that. We don't have the documentation cleaned up, however if we do not do CLUSTER here - well reporting table is not small, but also not critical, and nothing much happens without optimizing it. So I'd put the notice into the next release notes and call it a day.