Thread: contrib/adddepend does not properly re-create multi-column foreign keys.

Hallo,

I've used contrib/adddepend script (written in Perl) to move my
databases from PostgreSQL 7.2 to 7.3-style foreign key syntax, in
order to use new dependency information and get rid of those "create
constraint trigger" commands generated by pg_dump.  The script does a
very good job indeed.  It can be also downloaded from author's
website:

    http://www.rbt.ca/postgresql/upgrade.shtml

I noticed that both versions (attached to PostgreSQL 7.3 sources and
downloaded from author's website) have problems with multi-column
keys, which are not re-created properly.  For example, I had a table
of the following definition:

    create table foo (
        a integer not null,
        b integer not null,
        c integer,
        d integer,

        primary key (a, b),
        foreign key (c, d) references (a, b) match full
    );

After dumping this using pg_dump from PostgreSQL 7.2.3, and loading
the dump into PostgreSQL 7.3, adddepend script tried to create the
constraint with following command:

    alter table foo add foreign key (c, b) references (a, d) match full;

which didn't work of course, because columns b and d were swapped.

After some digging in the code, I found that when subroutine
"findForeignKeys" tries to parse trigger parameters, it loads first
two column names into $lcolumn_name and $fcolumn_name variables, and
all the rest into @junk array.  After this, it uses Perl pop()
function, each time twice, to get a pair of column names, and then
stores first pop'ped name as a foreign key column name and second as a
referenced column name.  This is wrong, because these names are
swapped this way.  Not only primary keys are swapped with foreign
keys, but the order of columns in a multi-column key is not preserved
either.  I think one should use shift() function instead, and it would
eliminate both problems.

Simply: the program takes column names from the wrong end of the array
:)

I already notified the author of the program and attached patches to
both original upgrade.pl and contrib/adddepend scripts.

Regards,

--
Adam Buraczewski <adamb@polbox.pl> * Linux registered user #165585
GCS/TW d- s-:+>+:- a- C+++(++++) UL++++$ P++ L++++ E++ W+ N++ o? K? w--
O M- V- PS+ !PE Y PGP+ t+ 5 X+ R tv- b+ DI? D G++ e+++>++++ h r+>++ y?

Adam Buraczewski <adamb@polbox.pl> writes:
> Simply: the program takes column names from the wrong end of the array
> :)

> I already notified the author of the program and attached patches to
> both original upgrade.pl and contrib/adddepend scripts.

Rod, do you concur that this is a correct patch?

            regards, tom lane

It makes sense, but I've not tested it yet.

On Sun, 2002-12-01 at 14:06, Tom Lane wrote:
> Adam Buraczewski <adamb@polbox.pl> writes:
> > Simply: the program takes column names from the wrong end of the array
> > :)
>
> > I already notified the author of the program and attached patches to
> > both original upgrade.pl and contrib/adddepend scripts.
>
> Rod, do you concur that this is a correct patch?

--
Rod Taylor <rbt@rbt.ca>

Rod Taylor <rbt@rbt.ca> writes:
> It makes sense, but I've not tested it yet.

Okay, I'll hold off applying until you check it.  It's not urgent,
but I'd like to have it fixed in 7.3.1, which we'll doubtless be
wanting to roll out in a few weeks.

            regards, tom lane

On Sun, Dec 01, 2002 at 02:06:31PM -0500, Tom Lane wrote:
> Adam Buraczewski <adamb@polbox.pl> writes:
> > I already notified the author of the program and attached patches to
> > both original upgrade.pl and contrib/adddepend scripts.
> Rod, do you concur that this is a correct patch?

The patched script worked properly for my 6 databases where I heavily
make use of two-column foreign keys.  However, I didn't have a chance
to check if it is correct for keys consisting of more than two
columns, and I also don't have good knowledge about parameters passed
to RI_FKey* procedures.  I based my "research" on analysis of
adddepend code.  Summarizing: it would be indeed good to verify this
patch before applying :)

Regards,

--
Adam Buraczewski <adamb@polbox.pl> * Linux registered user #165585
GCS/TW d- s-:+>+:- a- C+++(++++) UL++++$ P++ L++++ E++ W+ N++ o? K? w--
O M- V- PS+ !PE Y PGP+ t+ 5 X+ R tv- b+ DI? D G++ e+++>++++ h r+>++ y?

On Sun, 2002-12-01 at 14:11, Tom Lane wrote:
> Rod Taylor <rbt@rbt.ca> writes:
> > It makes sense, but I've not tested it yet.
>
> Okay, I'll hold off applying until you check it.  It's not urgent,
> but I'd like to have it fixed in 7.3.1, which we'll doubtless be
> wanting to roll out in a few weeks.

Adams patch is good.  Please apply when you find time.

--
Rod Taylor <rbt@rbt.ca>

Rod Taylor <rbt@rbt.ca> writes:
> Adams patch is good.  Please apply when you find time.

I'm a bit confused by the patch --- it has a patch against "upgrade.pl"
which is not to be seen in contrib/adddepend.  What's up with that?

            regards, tom lane

On Sun, Dec 01, 2002 at 06:57:25PM -0500, Tom Lane wrote:
> I'm a bit confused by the patch --- it has a patch against "upgrade.pl"
> which is not to be seen in contrib/adddepend.  What's up with that?

I attached _two_ patches to my first posting -- one for upgrade.pl and
one for adddepend.  Both patches are almost identical; only line
numbers change.  That's because after inclusion into PostgreSQL CVS
tree someone/something added comments at the beginning of the file.
And the name of the script was changed from original upgrade.pl to
adddepend, of course.

Besides, I generated both patches with "diff -u" command and with some
context around :)

Regards,

--
Adam Buraczewski <adamb@polbox.pl> * Linux registered user #165585
GCS/TW d- s-:+>+:- a- C+++(++++) UL++++$ P++ L++++ E++ W+ N++ o? K? w--
O M- V- PS+ !PE Y PGP+ t+ 5 X+ R tv- b+ DI? D G++ e+++>++++ h r+>++ y?

Adam Buraczewski <adamb@polbox.pl> writes:
> And the name of the script was changed from original upgrade.pl to
> adddepend, of course.

Oh, I see.  Looks like the README didn't get fixed for that change :-(
Will take care of it.

            regards, tom lane

On Sun, 2002-12-01 at 18:57, Tom Lane wrote:
> Rod Taylor <rbt@rbt.ca> writes:
> > Adams patch is good.  Please apply when you find time.
>
> I'm a bit confused by the patch --- it has a patch against "upgrade.pl"
> which is not to be seen in contrib/adddepend.  What's up with that?

It seems to be against the version I originally posted.

The second, adddepend.patch is what should be applied to the
contrib/adddepend/adddepend file

--
Rod Taylor <rbt@rbt.ca>

Rod Taylor <rbt@rbt.ca> writes:
> The second, adddepend.patch is what should be applied to the
> contrib/adddepend/adddepend file

Roger.  Patch applied in both HEAD and REL7_3_STABLE branches.

            regards, tom lane