There is bug in system.install in system.module. Column fid in table {files} should have been serial type for PostgreSQL database (line).

CommentFileSizeAuthor
#13 system.install_20061130_1640_patch.txt770 bytessammys
#12 system.install_9.patch2.85 KBhavran
#9 system.install_8.patch2.92 KBhavran
#7 system.install_7.patch2.86 KBhavran
#3 system.install_6.patch2.86 KBhavran
#1 system.install_5.patch815 byteshavran

Comments

havran’s picture

havran commented
Assigned: Unassigned » havran
Status: Active » Reviewed & tested by the community
StatusFileSize
new system.install_5.patch815 bytes

Here is patch for this issue.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

You need to create an update as well, for old installations.

Also, please don't mark your own patches "ready to be committed." They need community review.

havran’s picture

havran commented
StatusFileSize
new system.install_6.patch2.86 KB

Sorry. Ok i created update function (i look in function system_update_173() which is similar). Please check this. For my installation working ok, but i have only one record in files table (and file_revisions table).

pwolanin’s picture

pwolanin commented

Do you need to copy and recreate the table, rather than just doing an update for this one column definition?

havran’s picture

havran commented

I have PostgreSQL version 7.4. In PostgreSQL version < 8 there is no query like ALTER TABLE table ALTER COLUMN column TYPE newtype.

havran’s picture

havran commented

(ups i use >)

I have PostgreSQL version 7.4. In PostgreSQL version lower than 8 there is no query for altering column type.

havran’s picture

havran commented
Version: 5.x-dev » 5.0-beta2
Status: Needs work » Needs review
StatusFileSize
new system.install_7.patch2.86 KB

Still same bug in 5.0-beta2. Patch attached.

havran’s picture

havran commented

And now i have found same bug here http://drupal.org/node/97726?

havran’s picture

havran commented
StatusFileSize
new system.install_8.patch2.92 KB

I realise, we don't need any table operation on MySQL on this update. Modified patch attached.

chx’s picture

chx commented
Status: Needs review » Needs work

uh huh. what about adding a fid_serial column instead , then UPDATE so that fid_serial equals fid , then dropping fid columnd and renaming fid_serial finally to fid? At the end, you likely will want to VACUUM ANALYZE the poor table but still I would find this better than copying the whole thing around a few times.

havran’s picture

havran commented

(I use PostgreSQL 7.4)

For recreate fid as serial i must use:

DELETE FROM drupal_files WHERE fid=0;
DELETE FROM drupal_file_revisions WHERE fid=0;

ALTER TABLE drupal_files ADD COLUMN fid_serial integer;
UPDATE drupal_files SET fid_serial=fid;

ALTER TABLE drupal_files DROP COLUMN fid;
ALTER TABLE drupal_files RENAME COLUMN fid_serial TO fid;

ALTER TABLE drupal_files ALTER COLUMN fid SET NOT NULL;
ALTER TABLE drupal_files ADD CONSTRAINT drupal_files_fid CHECK (fid >= 0);
ALTER TABLE drupal_files ADD CONSTRAINT drupal_files_pkey PRIMARY KEY (fid);

CREATE SEQUENCE drupal_files_fid_seq;
SELECT setval('drupal_files_seq', max(fid)) FROM drupal_files;
ALTER TABLE drupal_files ALTER COLUMN fid SET DEFAULT nextval('drupal_files_fid_seq');

If column fid exists as serial i give error (i think it is possible). This is not very good. May i try drop drupal_files_fid_seq after droping previous fid column? I think recreate table through temporary table is simpler, safer and cleaner way for this job. And if i use transaction, there is no possibility for destroy {files} data. Maybe someone create better code?

Any other opinions?

havran’s picture

havran commented
Status: Needs work » Needs review
StatusFileSize
new system.install_9.patch2.85 KB

For safer execution I added TRANSACTION in patch. I have tested it on my installation without problem.

sammys’s picture

sammys commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new system.install_20061130_1640_patch.txt770 bytes

Ok... seems there is a lot of action going on in here. The patch is incorrect as the CHECK condition tests the nid field rather than fid. Another thing is that we're not supplying update code for this because 4.7 already has the serial column. Update code is, after all, making a transition from 4.7 to 5.0 and not 5.0 beta to 5.0 release.

Attached is the replacement patch for system.install. This is RTBC.

As for all you wonderful people out there using the beta release, please use the manual update SQL provided in this issue to update your {files} table.

--
Sammy Spets
Synerger
http://synerger.com

sammys’s picture

sammys commented

Oh and thank you for getting to the root of the problem. I screwed up in migrating the code across because I used the MySQL schema and mapped it over to PostgreSQL. There is no mention in there about the sequence needed for files table so it was stuffed. Apologies for the inconvenience.

sammys’s picture

sammys commented

One other thing while i'm here. The way you guys are deleting all entries from files and file_revisions may work, but it's only half the picture because node and node_revisions tables have entries in them as well. May I suggest using something like:

CREATE SEQUENCE files_fid_seq;
UPDATE files SET fid = nextval('files_fid_seq');
UPDATE file_revisions SET fid = files.fid FROM files, node_revisions WHERE files.nid = node_revisions.nid AND file_revisions.vid = node_revisions.vid;

Please take a backup of the files and file_revisions tables before performing these queries as i've only tested it with a small number of entries.

You shouldn't have too many entries in there anyway since the PRIMARY KEY constraint would have stopped you from having more than one fid == zero entry.

Cheers,
-- sammys

sammys’s picture

sammys commented

Duplicate issues:
http://drupal.org/node/97726
http://drupal.org/node/100486

RTBC so let's go! :)

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)