Avatar

Code review comments. (Development)

by Auge ⌂, Sunday, August 04, 2024, 10:59 (43 days ago) @ Joe I

Hello Joe,

Line 40/41 Update
Looks very good. ORDER BY Should generally work either with or without the “ft.”, however including "ft.time" Should eliminate any possible issue here, as you have done. See Line 109 for more info.

This was more to clarify, which column in which table is meant. In the query itself the relation is clear but this part of the query is generated to far away from the rest of the query to be self-explanatory.

Line 88 Update
I Think this looks good, but need to test numerous combinations. The original format should not have caused any issue according to SQL standards, however there have been some SQL / MySQL implementations that required any ORDER BY also be in the SELECT.

I would bet for a configuration issue. Be that as it may, on my webspace/database server the missing columns in the SELECT were reported as an error and adding it was the solution.

I need to make sure adding ft.sticky and ft.time in the SELECT DISTINCT won't change the # of output rows.

No, the change does not change the number of the lines in result. It doesn't because the thread-ID (`mlf2_entries.tid`), selected with DISTINCT in combination with WHERE ft.pid = 0 which ensure to select only the opening posting of a thread, is unique in every case and the additional selected columns doesn't change this in any case.

Line 109 Update
Looks good. The issue may stem from unsafe use of the variable name “time” in the SQL SELECT (Line 109), as this is a Reserved word.

I suspect that the conversion of the column time from the MySQL timestamp type to the Unix integer timestamp under the same name as the column itself makes sorting impossible. When adding the aliased column to the select and ordering by the alias (and so with the original values) the code works.

Some questions as well:

Q1: To avoid potential row result issues arising from the change in Line 88, I'd like to know if you did already try changing Line 40/41 without changing Line 88.

No, I didn't. I hope, my description above about DISTINCT in combination with the WHERE clause clarifies what happens.

From your upgrade notes:
"Only when I called up the main forum page with the overview of threads, which was also empty at this time, the page displayed a database error. It concerned queries with ORDER-BY clauses in which columns are to be sorted that are not contained in the SELECT."

Q2: Do you know if this error occurred on both Line 88 and 109?

Yes, it does. The first error message was for line #88. After solving the error, another error message came up, related to the query in line #109.

Q3: Can you confirm whether you're using MySQL or MariaDB? Trying to wrap my head around these, as I can't duplicate the issues on my existing system (MariaDB, with data). Still need to go through upgrade using an empty DB, but curious if there may be an additional complicating factor between MariaDB and MySQL.

I was able to reproduce the error on both systems, MySQL 8.x and MariaDB 10.5.x on the servers of one hosting company (with an obvioulsy strict configuration).

Testing factors I need to look at (particularly for Line 88 and system not crashing)
Categories (categories disabled, categories enabled but no categories, categories enabled and exist)
Spam (no spam entries, spam entries with view enabled & disabled)
MariaDB vs MySQL. I only have MariaDB 11, so won't be able to test out MySQL 8.

As mentioned in another posting I successfully upgraded a forum with several postings in several threads in different categories and with several users. The only scenario I did not test was to have spam postings in the data. But also these postings would only be postings. There was no change regarding their handling in an upgrade.

Tschö, Auge

--
Trenne niemals Müll, denn er hat nur eine Silbe!


Complete thread:

 RSS Feed of thread