DEV: Move the post and topic custom fields into a table #309

Lhcfl · 2024-09-05T09:58:52Z

This PR does some preparatory work to prepare for the front-end display of who marked an answer as solved. It migrates the custom fields from discourse-solved of Topic and Post to a new table, except "accepted_answer_post_id"

ref: /t/-/95318

This PR does some preparatory work to prepare for the front-end display of who marked an answer as solved. It migrates the custom fields from discourse-solved of Topic and Post to a new table, except "accepted_answer_post_id" ref: /t/-/95318

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb

nattsw · 2024-09-06T06:52:26Z

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb

+      FROM topic_custom_fields tc
+      LEFT JOIN topic_custom_fields tc2
+      ON tc2.topic_id = tc.topic_id AND tc2.name = 'solved_auto_close_topic_timer_id'
+      LEFT JOIN user_actions ua


Is there an index in user_actions for target_topic_id?

The query plan is:

QUERY PLAN ------------------------------------------------------------------------------------------------------------------------------- Unique (cost=14.85..14.87 rows=1 width=32) -> Sort (cost=14.85..14.86 rows=1 width=32) Sort Key: tc.topic_id, ((tc.value)::integer), ((tc2.value)::integer), ua.acting_user_id, tc.created_at, tc.updated_at -> Nested Loop (cost=0.42..14.84 rows=1 width=32) -> Nested Loop Left Join (cost=0.00..6.16 rows=1 width=84) Join Filter: (tc2.topic_id = tc.topic_id) -> Seq Scan on topic_custom_fields tc (cost=0.00..3.08 rows=1 width=52) Filter: ((name)::text = 'accepted_answer_post_id'::text) -> Seq Scan on topic_custom_fields tc2 (cost=0.00..3.08 rows=1 width=36) Filter: ((name)::text = 'solved_auto_close_topic_timer_id'::text) -> Index Only Scan using idx_unique_rows on user_actions ua (cost=0.42..8.66 rows=1 width=8) Index Cond: ((action_type = 15) AND (target_topic_id = tc.topic_id)) (12 rows)

The query is using idx_unique_rows on user_actions ua, so i think yeah, it is using an index

-> Seq Scan on topic_custom_fields tc (cost=0.00..3.08 rows=1 width=52) Filter: ((name)::text = 'accepted_answer_post_id'::text)

This looks interesting to me. I wonder if we need a index on name. From my rusty knowledge Seq Scan can be costly if the table is large and if there are no indexes to filter on (e.g. the accepted_answer_post_id)

Oh I just noticed that custom_fields does not an index on name... 😢

I find it interesting that we have no index on topic_custom_fields.name. Wondering if it makes sense to have one added in core.

We have this one:

# topic_custom_fields_value_key_idx (value,name) WHERE ((value IS NOT NULL) AND (char_length(value) < 400))

Which I find a bit strange, why not (name, value)?

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb

nattsw · 2024-09-06T12:08:21Z

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb

+    execute <<-SQL
+      DELETE FROM post_custom_fields
+      WHERE name = 'is_accepted_answer'
+    SQL
+
+    execute <<-SQL
+      DELETE FROM topic_custom_fields
+      WHERE name = 'solved_auto_close_topic_timer_id'
+    SQL


I think the deletions here are ok though..

... if we have better knowledge or data, and we know that there are many rows to be deleted, we can consider deletion in batches. Let me do a quick check on who may be a heavy user of discourse-solved...

Yeah ok looks like we do have some hundreds of thousands of rows (as mentioned in chat) so perhaps nice to batch.

I think there is a trick to make it a bit better:

DELETE FROM topic_custom_fields WHERE id IN (SELECT id FROM topic_custom_fields WHERE name = 'solved_auto_close_topic_timer_id')

... but it needs to be tested.

app/lib/plugin_initializers/assigned_reminder_exclude_solved.rb

plugin.rb

nbianca · 2024-09-06T13:43:11Z

plugin.rb

@@ -190,6 +182,12 @@ def self.skip_db?
    ::PostSerializer.prepend(DiscourseSolved::PostSerializerExtension)
    ::UserSummary.prepend(DiscourseSolved::UserSummaryExtension)
    ::Topic.attr_accessor(:accepted_answer_user_id)
+    ::Topic.has_one(:solution, class_name: ::DiscourseSolved::Solution.to_s)


I am wondering if this and the next line should be in a Extension module.

spec/fabricators/extend_topic_fabricator.rb

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb

nbianca · 2024-09-06T14:03:12Z

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb

+      FROM topic_custom_fields tc
+      LEFT JOIN topic_custom_fields tc2
+      ON tc2.topic_id = tc.topic_id AND tc2.name = 'solved_auto_close_topic_timer_id'
+      LEFT JOIN user_actions ua


I find it interesting that we have no index on topic_custom_fields.name. Wondering if it makes sense to have one added in core.

We have this one:

# topic_custom_fields_value_key_idx (value,name) WHERE ((value IS NOT NULL) AND (char_length(value) < 400))

Which I find a bit strange, why not (name, value)?

nbianca · 2024-09-06T14:06:11Z

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb

+    execute <<-SQL
+      DELETE FROM post_custom_fields
+      WHERE name = 'is_accepted_answer'
+    SQL
+
+    execute <<-SQL
+      DELETE FROM topic_custom_fields
+      WHERE name = 'solved_auto_close_topic_timer_id'
+    SQL


I think there is a trick to make it a bit better:

DELETE FROM topic_custom_fields WHERE id IN (SELECT id FROM topic_custom_fields WHERE name = 'solved_auto_close_topic_timer_id')

... but it needs to be tested.

Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

…scourse_solved_solutions.rb Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

pmusaraj · 2025-02-27T02:10:20Z

@nattsw I am guessing this PR should be closed?

nattsw · 2025-03-21T13:14:18Z

Closing to move ownership to #342 as Linca has ended her internship.

Related: - #309 - #341 Requires: - discourse/discourse#31954 This commit converts all use of post and topic custom fields into a dedicated table: - migration for copying custom field into table - swap app usage of custom fields to table This commit does not attempt to fix issues or optimise, and does not delete old data from custom fields _yet_.

Lhcfl marked this pull request as draft September 5, 2024 11:19

Lhcfl added 2 commits September 5, 2024 19:35

remove unused post/topic custom fields

56f8327

fix badges SQL

29f744e

nattsw reviewed Sep 6, 2024

View reviewed changes

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb Outdated Show resolved Hide resolved

nattsw reviewed Sep 6, 2024

View reviewed changes

remove down sql

c00ca89

Lhcfl marked this pull request as ready for review September 6, 2024 11:25

nattsw reviewed Sep 6, 2024

View reviewed changes

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb Show resolved Hide resolved

nattsw reviewed Sep 6, 2024

View reviewed changes

db/migrate/20240905024953_move_solved_topic_custom_field_to_discourse_solved_solutions.rb Outdated Show resolved Hide resolved

nattsw reviewed Sep 6, 2024

View reviewed changes

review

ba79689

nbianca reviewed Sep 6, 2024

View reviewed changes

Lhcfl and others added 5 commits September 10, 2024 14:25

Update app/lib/plugin_initializers/assigned_reminder_exclude_solved.rb

2c0aff5

Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

Update spec/fabricators/extend_topic_fabricator.rb

1a07c87

Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

Update db/migrate/20240905024953_move_solved_topic_custom_field_to_di…

15bc182

…scourse_solved_solutions.rb Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

Update db/migrate/20240905024953_move_solved_topic_custom_field_to_di…

d10c7c3

…scourse_solved_solutions.rb Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

review: remove old_solution

ca19bb7

nattsw mentioned this pull request Mar 21, 2025

DEV: Move solved custom fields into a table #342

Merged

nattsw closed this Mar 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEV: Move the post and topic custom fields into a table #309

DEV: Move the post and topic custom fields into a table #309

Lhcfl commented Sep 5, 2024

nattsw Sep 6, 2024

Lhcfl Sep 6, 2024

nattsw Sep 6, 2024

Lhcfl Sep 6, 2024

nbianca Sep 6, 2024

nattsw Sep 6, 2024 •

edited

Loading

nattsw Sep 6, 2024 •

edited

Loading

nbianca Sep 6, 2024

nbianca Sep 6, 2024

nbianca Sep 6, 2024

nbianca Sep 6, 2024

pmusaraj commented Feb 27, 2025

nattsw commented Mar 21, 2025

DEV: Move the post and topic custom fields into a table #309

DEV: Move the post and topic custom fields into a table #309

Conversation

Lhcfl commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nattsw Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

nattsw Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmusaraj commented Feb 27, 2025

nattsw commented Mar 21, 2025

nattsw Sep 6, 2024 •

edited

Loading

nattsw Sep 6, 2024 •

edited

Loading