Comment issues with beta4 on postgre

[skippy]

first off, this is a FANTASTIC piece of software, as everone is saying. I was using/supporting 4 different blogging packages, but with 0.8, I'm moving everyone over to your code. great work.

I noticed a few odd behaviors with comments:
- a comment will show up as Anonymous, even f they entered a name, etc. If you click the little link next to them, their name will show up in the reply dropdown for adding your own comment. I can understand why some blogs might want this to read anonymous, but I would think this should be a flaggable security feature
- from the main page, the # of comments that are show is incorrect. For the first few entries, it shows 0, even though 2-4 exist. However, an entry from a few weeks ago shows the correct # of comments. If you click on the entry, the comments do show up..the numbering is just wrong.

At first I thought it might have been some plug-in issues, so I deleted almost all of them, leaving just the min # of event plug-ins..and that didn't fix it. I noticed that when I removed them in the admin tool, it actually didn't delete the folder. I am guessing they are no longer used in any maner, but there could possibly still be a bad hook to the disabled plug-in?

The blog is:
bluegarter.greenemenagerie.org
Running on apache, using postgre.

Let me know if you would like any additional information. Sorry if this was brought up before..I scanned the forums and didn't see anything, but it is always possible that I missed something!
thank you again,
Adam

[garvinhicking]

First off, thanks a lot for your praise and feedback. Glad you like it!

The first bug with comments being shown as 'Anomyous' was a template problem of the Kubrick port. It would not appear in the other templates, and I've just committed a fix. You can just replace 'username' with 'author' in the templates/kubrick/comments.tpl file.

About the plugins: When you remove them, the files still exist because you might want to install them at a different time. We operate with simple database references that tell SErendipity if you use or do not use a plugin. Thus, if you've removed it in the DB, it will definitely not get executed. I swear.

The issue about wrong comment counts is hard for me to reproduce. It might be a postgresql issue, as it works on my MySQL machine. Can you check your postgresql error log and maybe watch for errors? If you have access to your DB, can you check your serendipity_entries table and see what number you see for the appropriate entry in the 'comments' column?

And on new entries it doesn't count the comments right as well? Did you import entries to your blog?

Regards,
Garvin

[skippy]

hey gavin,
doh! I just figured out the template thing...was hoping to save you a bit of time.

As for the entries...most of them existed when i upgraded, so they show the correct numbers. It is the new entries that don't. I can't figure out how to get access to the logs (I'm running on a hosted service and they don't let me have much access at all except for the php admin tool). Can I turn on a debug mode for serendipity, or have it print its own logs? I took a look at file: functions_comments.inc.php, ln: 334. I see an insert into the comments table, but I don't see any sort of update to the entries table to increment the 'comments' field by ++. Was there supposed to be a trigger to do this?

Unfortunately, I'm late to work(!!). If you have any additional insights, that would be great. I'll try and figure it out tonight.
thanks again!
adam

[garvinhicking]

Our system is like this, when a comment is added:

1. Run it through the spamblock plugin, if that exists. That may set a comment to moderate
2. Check if the entry you made has moderation turned on
3. If either 1. or 2. returned true, the comment is toggled with a "moderate" flag and only shown to you as an Admin.
4. The comment is inserted into the Database via include/functions_comments.inc.php, function serendipity_saveComment()
5. If moderation is NOT toggled for the entry, the function serendipity_approveComment() is called.
6. serendipity_approveComment() will also be called later if you approve a comment. That function takes care to increment the 'comments' counter of your serendipity_entries row. This is line 296 of the functions_comments.inc.php:

Code: Select all

    $query = "UPDATE {$serendipity['dbPrefix']}entries SET $field=$field+1, last_modified=". $lm ." WHERE id='". (int)$entry_id ."'";

Could you check if that causes any trouble for your postgresql setup? Sadly I don't have a pgsql setup available right now. But I think it has worked before! Maybe you can edit the script and add debugging there (echo $query) so that you can execute the code manually via phpPgAdmin or so.

Thanks for you willing to help us in the issue! It is much appreciated!

Regards,
Garvin

[skippy]

hey gavin,
Ok, I traced it down to one line of code.
First off, I couldn't get any echo or print statements to appear...so I printed to log. Then noticed that it wasn't getting to the update statement in
serendipity_approveComment
because the rs was returning false (nothing found) here

Code: Select all

    if ( $rs === false ) {
        return false;
    }

then the select clause above that was formed to look like this:

Code: Select all

SELECT c.*, e.title, a.email as authoremail, a.mail_comments, e.timestamp AS entry_timestamp, e.last_modified AS entry_last_modified
                FROM adamgre_bluegarterDbcomments c
                LEFT JOIN adamgre_bluegarterDbentries e ON (e.id = c.entry_id)
                LEFT JOIN adamgre_bluegarterDbauthors a ON (e.authorid = a.authorid)
                WHERE c.id = '9345289'

well, I don't have a c.id that high!
going down the stack to the next function is serendipity_saveComment.
Here, the cid is incorrectly created:

Code: Select all

        $cid = serendipity_db_insert_id('comments', 'id');

returns numbers like '9345289'

looking at the serendipity_db_insert_id function in the include/db/postgres.inc.php, it creates sql that fails

Code: Select all

SELECT currval('adamgre_bluegarterDbcomments_id_seq'::text) AS id

now I don't know postgre all that well (My experience with relational dbs tends to be more towards enterprise systems), but this seems a bit complicated when all you want is the max id value.
when I run this directly in phpPgAdmin, I see this

SQL error:

ERROR: pg_aclcheck: class "adamgre_bluegarterdbcomments_id_seq" not found

In statement:
SELECT COUNT(*) AS total FROM (SELECT currval('adamgre_bluegarterDbcomments_id_seq'::text) AS id) AS sub

why can't you just do this:

Code: Select all

SELECT max(id) from adamgre_bluegarterdbcomments

or if this function is supposed to show the next available id:
SELECT max(id) + 1 from adamgre_bluegarterdbcomments

or better yet, if postgre supports it, an auto increment column...that way you will increment it in one atomic unit of work, while here there is a chance of concurrency issues.

Well, hope that helps! I'm going to stick with the max(id) as I know that works, but an autoincrement col would be best.
thanks gavin!
adam[/code]

[skippy]

hey Gavin,
There might be a better way to do this. This function works for the test cases I was running against it, the blog still seems to function. Only more tests will tell....

Let me know if this is correct or if there is a better way
thanks!
adam

Code: Select all

function serendipity_db_insert_id($table = '', $id = '') {
    global $serendipity;
    if (empty($table) || empty($id)) {
        // BC - will/should never be called with empty parameters!
        return pg_last_oid($serendipity['dbLastResult']);
    } else {
        //$query = "SELECT currval('{$serendipity['dbPrefix']}{$table}_{$id}_seq'::text) AS {$id}";
        $query = "SELECT max({$id}) as {$id} from {$serendipity['dbPrefix']}{$table}";
        if($res === false){
            return pg_last_oid($serendipity['dbLastResult']); // BC - should not happen!
        } else {
            $insert_id = pg_fetch_array($res, 0, PGSQL_ASSOC);
            return $insert_id[$id];
        }
    }
}

[rickmans]

hey gavin,


or better yet, if postgre supports it, an auto increment column...that way you will increment it in one atomic unit of work, while here there is a chance of concurrency issues.
adam[/code]

postgresql has sequences which are auto incremented columns, the currval function is a function to get the current sequence number of a certain sequence.

[skippy]

hey gavin,
if leaving the code as is (which seems best if a sequence col is like an auto-increment, as rickman pointed out (thanks!), then it is important to note that the sequence adamgre_bluegarterdbcomments_id_seq was not built.
When I tried to build it (granted, through the phpPgAdmin tool), it limited the characters so it came out as: adamgre_bluegarterdbcomments_id
woops!

in my older version of serendipity, the sequence col was of the name: adamgre_serendipitydbcom_id_seq, which works!!

sooooo, for PostgreSQL 7.1.3, there is a definite name length conflict here.
I guess there are two things that can be done:
1) go back to making it com_id_seq, or
2) I reinstall my db, and make the qualifier shorter!

going with 2 is probably the easiest, though 1 might be a bit more flexible? At the very least, gavin, you might want to note that the newest version might break in postgre because of this quirk.

well, either way, let me know which way you go, and I'll make sure my code is consistent with what you have.
thanks!
adam

ps. I didn't see anywhere in the code where the sequences were constructed. A grep on _seq turned up almost nothing.

[garvinhicking]

Hi Skippy!

First off, I really don't know much postgresql. We use sequences because PostgreSQL does not support the autoincrement stuff like MySQL easily does.

Previously we used a pgsql OID concept, but that got deprecated in latest postgresql versions. Thus we advanced to the sequences concept.

PostgreSQL users told me that the sequences were created automatically usually on standard pgsql installations. I think that is the case because most people using pgsql have no problem with it.

I don't know why the sequence was not created for you. Maybe if you ever renamed your pgsql tables, the sequences got not renamed. For the character limit, you might want to use a smaller dbPrefix with less characters. Or complain to the pgsql guys about nasty char restrictions.

Serendipity has no influcence how the Sequences are called, because pgsql creates them automatically.

I'll put up a note for the final release that postgreSQL users should take care of their sequences

Thanks for your help in this issue

Regards,
Garvin

[skippy]

Hey gavin, I'll install a fresh copy with shorter dbPrefix names and copy the data over. Take 2!
So just note about the name limit that postgre users might hit.
thanks again!
best of luck,
Adam