New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sorts topics by message time rather than message id #3953

Closed
wants to merge 23 commits into
base: release-2.1
from

Conversation

Projects
None yet
8 participants
@Sesquipedalian
Member

Sesquipedalian commented Feb 28, 2017

Under normal circumstances, sorting by id works well enough. But if messages and topics were imported from another source, such as another forum database or via an RSS posting mod or something, it is possible that an older message will have a higher id number than a newer message. Since the user normally expects sorting by the first or last post to return a list sorted chronologically rather than by SMF's internal id numbers, this change simply makes sure that we deliver as expected in all cases.

I don't think there could be any negative consequences of this change to the message index sorting, but if anyone can think of any issues that it might raise, please point them out.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Feb 28, 2017

Member

Hm. I'm not sure what Travis is complaining about now. The log for the failed job is completely empty.

Member

Sesquipedalian commented Feb 28, 2017

Hm. I'm not sure what Travis is complaining about now. The log for the failed job is completely empty.

@Antes

This comment has been minimized.

Show comment
Hide comment
@Antes

Antes Feb 28, 2017

Contributor

@Sesquipedalian you forgot to sign-off your commit ?

Contributor

Antes commented Feb 28, 2017

@Sesquipedalian you forgot to sign-off your commit ?

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian
Member

Sesquipedalian commented Feb 28, 2017

D'oh!

Sorts topics by message time rather than message id
Under normal circumstances, sorting by id works well enough. But if messages and topics were imported from another source, such as another forum database or via an RSS posting mod or something, it is possible that an older message will have a higher id number than a newer message. Since the user normally expects sorting by the first or last post to return a list sorted chronologically rather than by SMF's internal id numbers, this change simply makes sure that we deliver as expected in all cases.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Antes

This comment has been minimized.

Show comment
Hide comment
@Antes

Antes Mar 1, 2017

Contributor

Its failing because of outage in AWS stuff (https://www.traviscistatus.com/).

Contributor

Antes commented Mar 1, 2017

Its failing because of outage in AWS stuff (https://www.traviscistatus.com/).

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Mar 1, 2017

Member

Kicked off a rebuild for you.

However we leave the default sort by ID as it is more effiicent in a database than other sorting methods. When you issue a ORDER BY, there is some work to be done and this on large forums this can really slow databases because of the manual sorting done in filesorting/temp tables.
https://dev.mysql.com/doc/refman/5.7/en/order-by-optimization.html

Member

jdarwood007 commented Mar 1, 2017

Kicked off a rebuild for you.

However we leave the default sort by ID as it is more effiicent in a database than other sorting methods. When you issue a ORDER BY, there is some work to be done and this on large forums this can really slow databases because of the manual sorting done in filesorting/temp tables.
https://dev.mysql.com/doc/refman/5.7/en/order-by-optimization.html

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Mar 1, 2017

Collaborator

yeah it's a better idea to keep the sort by id.
your should rewrite the converter to fit this,
as to break smf.

Collaborator

albertlast commented Mar 1, 2017

yeah it's a better idea to keep the sort by id.
your should rewrite the converter to fit this,
as to break smf.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 1, 2017

Member

What if we added two more indexed columns to the topics table called first_msg_time and last_msg_time? Then we could issue an ORDER BY against those columns in the same way that we already can against id_first_msg and id_last_msg, right? We'd need to add some code to update these values in the topics table when new posts are created, but that won't be hard.

Member

Sesquipedalian commented Mar 1, 2017

What if we added two more indexed columns to the topics table called first_msg_time and last_msg_time? Then we could issue an ORDER BY against those columns in the same way that we already can against id_first_msg and id_last_msg, right? We'd need to add some code to update these values in the topics table when new posts are created, but that won't be hard.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Mar 1, 2017

Collaborator

Why you not update the id_topic so that it in the right order,
or why you not insert the data in the right order?

Why we try to fix something what is outside?

Collaborator

albertlast commented Mar 1, 2017

Why you not update the id_topic so that it in the right order,
or why you not insert the data in the right order?

Why we try to fix something what is outside?

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 1, 2017

Member

Topic ids should never, ever change once they are created. For example, if someone shares a link to http://www.example.com/forum/index.php?topic=78.0, but then we renumber the topic ids, that link will point to the wrong topic. Also, many other parts of the code refer to topics by id and store topic ids in other tables. Renumbering topic ids is not a good idea.

As for inserting data in the right order, I'm talking about when data is imported from other sources by various means. When we add a new topic from another source into our database, we need to give it an incremented id number. But the content of that topic may be minutes, hours, or years old. It does not make sense to present a three year old topic as though it were "newer" than a three day old topic, even if the three year old topic was imported into our database after the three day old topic.

Member

Sesquipedalian commented Mar 1, 2017

Topic ids should never, ever change once they are created. For example, if someone shares a link to http://www.example.com/forum/index.php?topic=78.0, but then we renumber the topic ids, that link will point to the wrong topic. Also, many other parts of the code refer to topics by id and store topic ids in other tables. Renumbering topic ids is not a good idea.

As for inserting data in the right order, I'm talking about when data is imported from other sources by various means. When we add a new topic from another source into our database, we need to give it an incremented id number. But the content of that topic may be minutes, hours, or years old. It does not make sense to present a three year old topic as though it were "newer" than a three day old topic, even if the three year old topic was imported into our database after the three day old topic.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Mar 1, 2017

Collaborator

The importing of data from "outside" is no function which smf support,
so is this use case not supported by smf -> smf is wrong side to fix this issue.

Collaborator

albertlast commented Mar 1, 2017

The importing of data from "outside" is no function which smf support,
so is this use case not supported by smf -> smf is wrong side to fix this issue.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 1, 2017

Member

There are mods that can bring this situation about, as well as various converting, importing, and merging scripts. But how exactly one could end up with topics whose id numbers and first/last post times do not coincide is beside the point. The point is this: if the user says she wants the topics to be sorted chronologically, we should sort them chronologically.

So I return to my question: if we added two more indexed columns to the topics table called first_msg_time and last_msg_time, and then sort based on those values rather than on id_topic and id_last_msg, does anyone see any reason why that would not work?

Member

Sesquipedalian commented Mar 1, 2017

There are mods that can bring this situation about, as well as various converting, importing, and merging scripts. But how exactly one could end up with topics whose id numbers and first/last post times do not coincide is beside the point. The point is this: if the user says she wants the topics to be sorted chronologically, we should sort them chronologically.

So I return to my question: if we added two more indexed columns to the topics table called first_msg_time and last_msg_time, and then sort based on those values rather than on id_topic and id_last_msg, does anyone see any reason why that would not work?

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Mar 1, 2017

Member

id_first_msg and id_last_msg are primarily used for being able to retrieve those messages quickly for things such as the MessageIndex where we want to display the first and last message details. They are not used in sorting. Using MIN/MAX here is slower. We only use MIN/MAX during a rebuild/recount on forum maintenance.

If this is happening because of a conversion, the converter should do the ORDER BY itself. Its better to let it do it and eat the extra temp tables (/filesorts) during the conversion than all the time.

Member

jdarwood007 commented Mar 1, 2017

id_first_msg and id_last_msg are primarily used for being able to retrieve those messages quickly for things such as the MessageIndex where we want to display the first and last message details. They are not used in sorting. Using MIN/MAX here is slower. We only use MIN/MAX during a rebuild/recount on forum maintenance.

If this is happening because of a conversion, the converter should do the ORDER BY itself. Its better to let it do it and eat the extra temp tables (/filesorts) during the conversion than all the time.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 2, 2017

Member

id_first_msg and id_last_msg are primarily used for being able to retrieve those messages quickly for things such as the MessageIndex where we want to display the first and last message details. They are not used in sorting. Using MIN/MAX here is slower. We only use MIN/MAX during a rebuild/recount on forum maintenance.

No, id_last_msg is the default sort key used when displaying the message index. See here. We ORDER BY is_sticky and then whatever is set in $_REQUEST['sort'], which is usually id_last_msg. See here.

If this is happening because of a conversion, the converter should do the ORDER BY itself. Its better to let it do it and eat the extra temp tables (/filesorts) during the conversion than all the time.

There are a number of ways that this could happen. I stumbled across the issue when testing out an RSS posting bot mod that preserved the original publication date of the posts it created. The posts would correctly be dated to, say, 2012 or whenever, but in the message index they would be sorted into positions that made them appear to be newer than posts from last week. This situation had nothing to do with the RSS posting bot process itself, and everything to do with the way the MessageIndex.php orders the topics by id number rather than by date. Any other process that adjusts the dates on posts could produce a similar situation. So long as MessageIndex.php sorts topics by id when asked to sort them by date, the potential for this discrepancy exists.

The point is that regardless of how, or even if, a backdated topic was added to the database, we want the message index to sort the topics chronologically when the user requests that they be sorted chronologically. There is no good way to guarantee that will happen if we are actually sorting by id numbers.

Member

Sesquipedalian commented Mar 2, 2017

id_first_msg and id_last_msg are primarily used for being able to retrieve those messages quickly for things such as the MessageIndex where we want to display the first and last message details. They are not used in sorting. Using MIN/MAX here is slower. We only use MIN/MAX during a rebuild/recount on forum maintenance.

No, id_last_msg is the default sort key used when displaying the message index. See here. We ORDER BY is_sticky and then whatever is set in $_REQUEST['sort'], which is usually id_last_msg. See here.

If this is happening because of a conversion, the converter should do the ORDER BY itself. Its better to let it do it and eat the extra temp tables (/filesorts) during the conversion than all the time.

There are a number of ways that this could happen. I stumbled across the issue when testing out an RSS posting bot mod that preserved the original publication date of the posts it created. The posts would correctly be dated to, say, 2012 or whenever, but in the message index they would be sorted into positions that made them appear to be newer than posts from last week. This situation had nothing to do with the RSS posting bot process itself, and everything to do with the way the MessageIndex.php orders the topics by id number rather than by date. Any other process that adjusts the dates on posts could produce a similar situation. So long as MessageIndex.php sorts topics by id when asked to sort them by date, the potential for this discrepancy exists.

The point is that regardless of how, or even if, a backdated topic was added to the database, we want the message index to sort the topics chronologically when the user requests that they be sorted chronologically. There is no good way to guarantee that will happen if we are actually sorting by id numbers.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Mar 2, 2017

Member

Well I guess I was wrong on how MessageIndex handles it. However id_first_msg and id_last_msg also contain a index, which helps prevent the file sort.

Member

jdarwood007 commented Mar 2, 2017

Well I guess I was wrong on how MessageIndex handles it. However id_first_msg and id_last_msg also contain a index, which helps prevent the file sort.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 2, 2017

Member

Yup. Which is why I am thinking that I will revert the current changes in this PR and instead add first_msg_time and last_msg_time columns and index them.

Member

Sesquipedalian commented Mar 2, 2017

Yup. Which is why I am thinking that I will revert the current changes in this PR and instead add first_msg_time and last_msg_time columns and index them.

Tracks first/last message times in topics table
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 7, 2017

Member

I'm interested to @albertlast's opinion on whether this is the best way to index the first_msg_time and last_msg_time columns. Both columns are written to at the same time, but only one or the other is read in any given query.

Member

Sesquipedalian commented Mar 7, 2017

I'm interested to @albertlast's opinion on whether this is the best way to index the first_msg_time and last_msg_time columns. Both columns are written to at the same time, but only one or the other is read in any given query.

Adds a maint. task to fix unset first/last msg times in topics table
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Mar 8, 2017

Collaborator

Well the first issue that your update syntax is wrong for pg:
http://stackoverflow.com/questions/7869592/how-to-do-an-update-join-in-postgresql
https://www.postgresql.org/docs/9.1/static/sql-update.html

Exp:

UPDATE smf_topics AS t
SET t.first_msg_time = mf.poster_time, t.last_msg_time = ml.poster_time
from smf_messages AS mf, smf_messages AS ml
WHERE
t.id_first_msg = mf.id_msg
and t.id_last_msg = ml.id_msg
and (t.first_msg_time = 0 OR t.last_msg_time = 0)

Personaly i like more the "with" syntax which is shown in the stackoverflow

The is in my eyes no other possibiltiy at the index topic.

Collaborator

albertlast commented Mar 8, 2017

Well the first issue that your update syntax is wrong for pg:
http://stackoverflow.com/questions/7869592/how-to-do-an-update-join-in-postgresql
https://www.postgresql.org/docs/9.1/static/sql-update.html

Exp:

UPDATE smf_topics AS t
SET t.first_msg_time = mf.poster_time, t.last_msg_time = ml.poster_time
from smf_messages AS mf, smf_messages AS ml
WHERE
t.id_first_msg = mf.id_msg
and t.id_last_msg = ml.id_msg
and (t.first_msg_time = 0 OR t.last_msg_time = 0)

Personaly i like more the "with" syntax which is shown in the stackoverflow

The is in my eyes no other possibiltiy at the index topic.

@albertlast
  • SQL Syntax for pg doesn't work
  • recommandation use for mysql bigint without unsigned like for pg (than they got the same min/max range)
Show outdated Hide outdated Sources/MoveTopic.php Outdated
Show outdated Hide outdated Sources/RemoveTopic.php Outdated
Show outdated Hide outdated Sources/RemoveTopic.php Outdated
Show outdated Hide outdated Sources/RemoveTopic.php Outdated
Show outdated Hide outdated Sources/RemoveTopic.php Outdated
Show outdated Hide outdated Sources/ScheduledTasks.php Outdated
Show outdated Hide outdated Sources/SplitTopics.php Outdated
Show outdated Hide outdated Sources/SplitTopics.php Outdated
Show outdated Hide outdated Sources/SplitTopics.php Outdated
Show outdated Hide outdated Sources/Subs-Post.php Outdated
@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 8, 2017

Member

Thanks, @albertlast.

  1. Will the syntax you provided also work for MySQL? We need something that will work for both, of course.
  2. We might need to find and fix some other queries as well. I modelled this query on another I found already in the code. Never mind. My memory failed me. :)
Member

Sesquipedalian commented Mar 8, 2017

Thanks, @albertlast.

  1. Will the syntax you provided also work for MySQL? We need something that will work for both, of course.
  2. We might need to find and fix some other queries as well. I modelled this query on another I found already in the code. Never mind. My memory failed me. :)
Postgres compatibility
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 8, 2017

Member

Instead of trying to find a syntax that would work for both MySQL and PostgreSQL, I've just changed my code so that it builds the query differently for each case.

Member

Sesquipedalian commented Mar 8, 2017

Instead of trying to find a syntax that would work for both MySQL and PostgreSQL, I've just changed my code so that it builds the query differently for each case.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 8, 2017

Member

recommandation use for mysql bigint without unsigned like for pg (than they got the same min/max range)

I have no reason to disagree with you that this would be better. I chose to use unsigned INT for MySQL only because I wanted these columns to exactly match poster_time in the messages table.

If you think a signed BIGINT column would be better for storing Unix timestamps, probably the best idea would be for you to submit a PR to change that in all the columns that store that kind of value.

Member

Sesquipedalian commented Mar 8, 2017

recommandation use for mysql bigint without unsigned like for pg (than they got the same min/max range)

I have no reason to disagree with you that this would be better. I chose to use unsigned INT for MySQL only because I wanted these columns to exactly match poster_time in the messages table.

If you think a signed BIGINT column would be better for storing Unix timestamps, probably the best idea would be for you to submit a PR to change that in all the columns that store that kind of value.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Mar 8, 2017

Collaborator

Thanks, @albertlast.

Will the syntax you provided also work for MySQL? We need something that will work for both, of course.
We might need to find and fix some other queries as well. I modelled this query on another I found already in the code. Never mind. My memory failed me. :)

there is no commone sql string which is supported by both
The only clean solution would be to create a update function db layer which do the magic.
But this would many effort i think,
so your solution could be better.

The bigint is the think,
i find this to many work to changes this in effery field (would by huge impact on updater scriper),
so only for new stuff we could look that we got the same technical limites on both side.

Collaborator

albertlast commented Mar 8, 2017

Thanks, @albertlast.

Will the syntax you provided also work for MySQL? We need something that will work for both, of course.
We might need to find and fix some other queries as well. I modelled this query on another I found already in the code. Never mind. My memory failed me. :)

there is no commone sql string which is supported by both
The only clean solution would be to create a update function db layer which do the magic.
But this would many effort i think,
so your solution could be better.

The bigint is the think,
i find this to many work to changes this in effery field (would by huge impact on updater scriper),
so only for new stuff we could look that we got the same technical limites on both side.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Mar 8, 2017

Contributor

I admit I haven't been following this thread closely, but at face value, I'd say embedding the different syntaxes in the code strays from SMF coding standards.

The purposes of the smcFuncs are to do exactly the magic described above - an abstracted DB function layer - you write the updates once with a call to smcFunc, & the differences are dealt with the different MySql & PG implementations (in Subs-Db-*.php, DbPackages-*.php & DbExtra-*.php).

I would look very closely at these existing functions first. If a specific function is missing, then add it... But I would not recommend embedding db-specific logic in there.

My 2 cents... It's possible I'm off base, I admit I'm still learning, but this sounded unusual given SMF's excellent library of functions for this very purpose...

Contributor

sbulen commented Mar 8, 2017

I admit I haven't been following this thread closely, but at face value, I'd say embedding the different syntaxes in the code strays from SMF coding standards.

The purposes of the smcFuncs are to do exactly the magic described above - an abstracted DB function layer - you write the updates once with a call to smcFunc, & the differences are dealt with the different MySql & PG implementations (in Subs-Db-*.php, DbPackages-*.php & DbExtra-*.php).

I would look very closely at these existing functions first. If a specific function is missing, then add it... But I would not recommend embedding db-specific logic in there.

My 2 cents... It's possible I'm off base, I admit I'm still learning, but this sounded unusual given SMF's excellent library of functions for this very purpose...

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Mar 8, 2017

Member

I looked through the existing $smcFunc db-* functions, and I didn't find anything to do this job. One could in theory try to abstract it all into a db-update function, but as @albertlast rightly said, that would take a lot of effort to do correctly. Such a function would either need to be restricted to handling only simple updates (in which case it is hardly worth doing) or else be able to handle complex updates (and my knowledge of SQL is not nearly strong enough to create that).

Member

Sesquipedalian commented Mar 8, 2017

I looked through the existing $smcFunc db-* functions, and I didn't find anything to do this job. One could in theory try to abstract it all into a db-update function, but as @albertlast rightly said, that would take a lot of effort to do correctly. Such a function would either need to be restricted to handling only simple updates (in which case it is hardly worth doing) or else be able to handle complex updates (and my knowledge of SQL is not nearly strong enough to create that).

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Mar 8, 2017

Contributor

Which part of the syntax isn't supported by both pg & mysql? I don't think I see it... Inner joins are supported by both... That's the most obscure part in there that I see...

pg is a lot more strict, & tends to only support ANSI standards. MySQL usually operates ANSI, but adds a lot of goofy stuff.

Contributor

sbulen commented Mar 8, 2017

Which part of the syntax isn't supported by both pg & mysql? I don't think I see it... Inner joins are supported by both... That's the most obscure part in there that I see...

pg is a lot more strict, & tends to only support ANSI standards. MySQL usually operates ANSI, but adds a lot of goofy stuff.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Mar 9, 2017

Collaborator

okay i see that datetime is hard to use,
because design error of poster_time...
and many other places where unixtimestamp is used instead of datetime.

I only want to mention how to convert a unixtimestamp to datetime
mysql FROM_UNIXTIME(1447430881)
pg to_timestamp(1447430881)

Collaborator

albertlast commented Mar 9, 2017

okay i see that datetime is hard to use,
because design error of poster_time...
and many other places where unixtimestamp is used instead of datetime.

I only want to mention how to convert a unixtimestamp to datetime
mysql FROM_UNIXTIME(1447430881)
pg to_timestamp(1447430881)

Sesquipedalian added some commits Mar 13, 2017

Improves legibility
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Properly passes $connection through to smf_db_query
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Corrects @param documentation
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Blober-Alex146

This comment has been minimized.

Show comment
Hide comment
@Blober-Alex146

Blober-Alex146 Jun 20, 2017

I may come late but what about this PR?

It could be good to sort topic by message time.

Also are you talking about sorting it by last topic activity or just last created topic

I see it "Merged" but there is some commits after this. So what's the state of this?

Blober-Alex146 commented Jun 20, 2017

I may come late but what about this PR?

It could be good to sort topic by message time.

Also are you talking about sorting it by last topic activity or just last created topic

I see it "Merged" but there is some commits after this. So what's the state of this?

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jun 20, 2017

Member

Also are you talking about sorting it by last topic activity or just last created topic?

Both.

Member

Sesquipedalian commented Jun 20, 2017

Also are you talking about sorting it by last topic activity or just last created topic?

Both.

@joshuaadickerson

This comment has been minimized.

Show comment
Hide comment
@joshuaadickerson

joshuaadickerson Jul 27, 2017

Contributor

The RSS posting bot should have gone to the end of the feed and then worked its way forward. As everyone else has said, the converter should reorder the posts and you should figure out how to forward broken links.

I can see a reason to backdate posts though.

If you're going to do that, you should change all the sorting to use that. Not sure if I saw it, but then your indexes should be {date, id}. You sort by date and then use the id as a tie-breaker.

I don't think this big of a change is worth it for your tiny use-case.

Contributor

joshuaadickerson commented Jul 27, 2017

The RSS posting bot should have gone to the end of the feed and then worked its way forward. As everyone else has said, the converter should reorder the posts and you should figure out how to forward broken links.

I can see a reason to backdate posts though.

If you're going to do that, you should change all the sorting to use that. Not sure if I saw it, but then your indexes should be {date, id}. You sort by date and then use the id as a tie-breaker.

I don't think this big of a change is worth it for your tiny use-case.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jul 27, 2017

Member

The RSS posting bot should have gone to the end of the feed and then worked its way forward. As everyone else has said, the converter should reorder the posts and you should figure out how to forward broken links.

That wouldn't make any difference. And the functioning of RSS posting bot is beside the point here, anyway. I only mentioned it to explain how I stumbled on this issue.

The issue is that when we say we are sorting chronologically, we should actually sort chronologically.

I can see a reason to backdate posts though.
If you're going to do that, you should change all the sorting to use that.

That's what this PR does.

Member

Sesquipedalian commented Jul 27, 2017

The RSS posting bot should have gone to the end of the feed and then worked its way forward. As everyone else has said, the converter should reorder the posts and you should figure out how to forward broken links.

That wouldn't make any difference. And the functioning of RSS posting bot is beside the point here, anyway. I only mentioned it to explain how I stumbled on this issue.

The issue is that when we say we are sorting chronologically, we should actually sort chronologically.

I can see a reason to backdate posts though.
If you're going to do that, you should change all the sorting to use that.

That's what this PR does.

@illori

This comment has been minimized.

Show comment
Hide comment
@illori

illori Sep 18, 2017

Contributor

are we going to do something with this PR?

Contributor

illori commented Sep 18, 2017

are we going to do something with this PR?

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Sep 22, 2017

Member

I hope so. But it isn't the sort of thing that I would want to merge unilaterally. I'm still waiting for some feedback from other official devs.

Member

Sesquipedalian commented Sep 22, 2017

I hope so. But it isn't the sort of thing that I would want to merge unilaterally. I'm still waiting for some feedback from other official devs.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Sep 22, 2017

Collaborator

The key issue on this pr is,
that it not fix a smf issue.

But create workaround for parts which are not connected with smf,
also the reason that topic_id can not be changed because urls didn't work any more,
is not so good.

Because with use of smf the other parts of the url change any way, too --> so old url never work after this migration.

Would be much more reason able, to add a col(by your "mod") with the old_topic_id so when you call
the old format url version that the mapping can be realized.

Collaborator

albertlast commented Sep 22, 2017

The key issue on this pr is,
that it not fix a smf issue.

But create workaround for parts which are not connected with smf,
also the reason that topic_id can not be changed because urls didn't work any more,
is not so good.

Because with use of smf the other parts of the url change any way, too --> so old url never work after this migration.

Would be much more reason able, to add a col(by your "mod") with the old_topic_id so when you call
the old format url version that the mapping can be realized.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Sep 25, 2017

Member

I still disagree with your interpretation of the situation, @albertlast. The issue is not caused by the mod in question. The mod is just how I discovered the issue. The issue instead is that when we claim to be sorting topics chronologically, we are actually sorting them by id.

Member

Sesquipedalian commented Sep 25, 2017

I still disagree with your interpretation of the situation, @albertlast. The issue is not caused by the mod in question. The mod is just how I discovered the issue. The issue instead is that when we claim to be sorting topics chronologically, we are actually sorting them by id.

Sesquipedalian added some commits Sep 28, 2017

Fixes #4337
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Removes topic time updates from daily maintenance
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Better handles repairing topic first/last_msg_time in RepairBoards
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Fixes a check for incorrect first/last message ids in a topic
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Avoids using separate queries to handle topic first/last_msg_time
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Takes advantage of first/last_msg_time in topic display
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@@ -640,11 +595,13 @@ function fix_full_processing:
MIN(ma.id_msg) END ELSE
MIN(mu.id_msg) END AS myid_first_msg,
CASE WHEN MAX(ma.id_msg) > 0 THEN MAX(ma.id_msg) ELSE MIN(mu.id_msg) END AS myid_last_msg,
t.first_msg_time, t.last_msg_time, mf.poster_time AS first_poster_time, ml.poster_time AS last_poster_time,

This comment has been minimized.

@albertlast

albertlast Sep 30, 2017

Collaborator

This col are missing in the group by part

@albertlast

albertlast Sep 30, 2017

Collaborator

This col are missing in the group by part

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Sep 30, 2017

Collaborator

Please had in mind that we support mysql below 5.7 so you need maintain the complet list of columns in group by and
not only the unfunction dep.

Collaborator

albertlast commented Sep 30, 2017

Please had in mind that we support mysql below 5.7 so you need maintain the complet list of columns in group by and
not only the unfunction dep.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Oct 20, 2017

Member

I'm closing this PR now. I would have liked to see this change make it in, but if we are trying to get a release candidate out, it is too late. Maybe in SMF 3.0

Member

Sesquipedalian commented Oct 20, 2017

I'm closing this PR now. I would have liked to see this change make it in, but if we are trying to get a release candidate out, it is too late. Maybe in SMF 3.0

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 20, 2017

Contributor

Sorry about that. A lot of work...

Probably a good call for the reason you stated.

Contributor

sbulen commented Oct 20, 2017

Sorry about that. A lot of work...

Probably a good call for the reason you stated.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Oct 20, 2017

Member

Yeah, I actually did a bunch more work that I never pushed up to GitHub, in order to handle splitting and merging, tracking unread messages, etc., properly using timestamps rather than ID numbers. It's nearly done, as a matter of fact, but as I looked at it I realized that it had turned into a significant overhaul—too much for this stage in the game.

I may turn it into a mod for the 2.1 branch. It won't be very interesting for end users by itself, but it will make things like importing, scheduled posts, backdating, etc., possible for other mods to implement.

Member

Sesquipedalian commented Oct 20, 2017

Yeah, I actually did a bunch more work that I never pushed up to GitHub, in order to handle splitting and merging, tracking unread messages, etc., properly using timestamps rather than ID numbers. It's nearly done, as a matter of fact, but as I looked at it I realized that it had turned into a significant overhaul—too much for this stage in the game.

I may turn it into a mod for the 2.1 branch. It won't be very interesting for end users by itself, but it will make things like importing, scheduled posts, backdating, etc., possible for other mods to implement.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 20, 2017

Member

Thanks. I agree it would be a big overhaul to do this. Mostly because everything in SMF is designed around the id rather than the timestamp.

As well this could cause unforeseen problems as timestamps are known for being wrong. Case in point, the server has the wrong time, the admin fixes it up (and or gets ntp working) and now the time is drifting to correct itself and messages are now sorted wrong. At least with id based sorting, even if the time is wrong, the messages are in the correct order.

Member

jdarwood007 commented Oct 20, 2017

Thanks. I agree it would be a big overhaul to do this. Mostly because everything in SMF is designed around the id rather than the timestamp.

As well this could cause unforeseen problems as timestamps are known for being wrong. Case in point, the server has the wrong time, the admin fixes it up (and or gets ntp working) and now the time is drifting to correct itself and messages are now sorted wrong. At least with id based sorting, even if the time is wrong, the messages are in the correct order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment