eulerto / pgquarrel

pgquarrel compares PostgreSQL database schemas (DDL)
BSD 3-Clause "New" or "Revised" License
388 stars 42 forks source link

Unnecessary no-op ALTER TRIGGER ... RENAME are emitted #39

Closed rafalcieslak closed 6 years ago

rafalcieslak commented 6 years ago

Steps to reproduce:

  1. On a new database, create a table and a procedure, create any trigger
  2. Compare the database with itself

I would expect empty output since a database is always identical to itself.

However, pgquarrel responds with:

ALTER TRIGGER trigger_name ON public.table_name RENAME TO trigger_name;

which is redundant, because the suggested rename does not change the name.


The reason for this behaviour is visible here in quarrel.c, extract below:

else if (compareNamesAndRelations(&triggers1[i].table, &triggers2[j].table,
                                  triggers1[i].trgname, triggers2[j].trgname) == 0)
{
    logDebug("trigger %s.%s: server1 server2", triggers1[i].table.schemaname,
             triggers1[i].table.objectname);
    dumpAlterTrigger(fpre, &triggers1[i], &triggers2[j]);
    i++;
    j++;
}

When a trigger is present in both databases, dumpAlterTrigger is called so that it can correct internal differences between these triggers. This makes sense, as this pattern is used very consistently in the entire codebase

However dumpAlterTrigger from trigger.c emits ALTER command unconditionally:

void
dumpAlterTrigger(FILE *output, PQLTrigger *a, PQLTrigger *b)
{
    char    *trgname1 = formatObjectIdentifier(a->trgname);
    char    *trgname2 = formatObjectIdentifier(b->trgname);
    char    *schema2 = formatObjectIdentifier(b->table.schemaname);
    char    *tabname2 = formatObjectIdentifier(b->table.objectname);

    fprintf(output, "\n\n");
    fprintf(output, "ALTER TRIGGER %s ON %s.%s RENAME TO %s;", trgname1, schema2, tabname2, trgname2);

    /* comment */
    if (options.comment)
    {
        [...]

So, in fact, when an identical trigger is present in both databases, a no-op ALTER TRIGGER ... RENAME is still produced.


Proposed solution:

I suggest wrapping the two fprint calls that emit the ALTER TRIGGER ... RENAME with a test whether the rename is actually required:

if (strcmp(trgname1, trgname2) != 0)
{
    fprintf(output, "\n\n");
    fprintf(output, "ALTER TRIGGER %s ON %s.%s RENAME TO %s;", trgname1, schema2, tabname2, trgname2);
}