Skip to content
This repository has been archived by the owner on Mar 28, 2019. It is now read-only.

Replace DROP INDEX IF EXISTS by a DO block #487

Merged
merged 2 commits into from
Oct 22, 2015

Conversation

rodo
Copy link
Contributor

@rodo rodo commented Oct 19, 2015

It's often a bad idea to DROP an index and recreate it just after to ensure the script is idempotent.
Even if the CREATE INDEX IF NOT EXISTS will be available soon in PostgreSQL 9.5 I suggest to replace the DROP INDEX instructions

AND tablename = 'records'
) THEN

CREATE UNIQUE INDEX idx_records_parent_id_collection_id_last_modified
ON records(parent_id, collection_id, last_modified DESC);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/me wondering: Since we are in a DO block, couldn't we catch the creation exception instead (less verbose) ?

(It might fill the server logs with meaningless errors though)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean exactly, by now if the index is not exists it will be created, and if it exists nothing will happen. I fixed an error in rodo@1477c25 suppose it's related on what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, what I mean is that there are two ways to write the same behaviour.
For example, in python:

if key in mydict:
    print mydict[key]

is equivalent (and less performant btw) to:

try:
    print mydict[key]
except KeyError:
    pass

So for the index creation I guess it's the same.
Either :

DO $$
BEGIN
    IF NOT EXISTS ( ... )
    THEN
         CREATE INDEX
    END IF;
END$$;

or

DO $$
BEGIN
    BEGIN
         CREATE INDEX ...
    EXCEPTION WHEN <error> THEN
            RAISE NOTICE 'Index exists with this name';
    END;
END$$;

Indeed, it would have prevented the typo you add in the commit you mentionned :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can do that, but the log will be more verbose as at each time you'll execute the file on your DB you'll have an error, I think we have a different goal. As a DBA I always want to reduce log amount, I prefer script that run without raising error, even if I have less information, here you want to Raise an error if you try to create an already existing index, the opposite side of view.
We have to define the goal before choosing a solution.
Be careful of the line number 4 that set min_loglevel to ERROR you won't see the NOTICE

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log will be more verbose

Yes, that what I thought in my first comment :)

This works for me ! I just wanted to challenge your choice :)

Thanks for the details !

@leplatrem
Copy link
Contributor

\o/ Thanks for this awesome contribution !

You can add your name to CONTRIBUTORS.rst :) We maintain it by hand.

@leplatrem
Copy link
Contributor

I realize that we should mimic the same improvements in the other backends cliquet/cache/postgresql/schema.sql and cliquet/permissions/postgresql/schema.sql

Idempotence for the win!

leplatrem added a commit that referenced this pull request Oct 22, 2015
Replace DROP INDEX with conditional creation in PostgreSQL backends
@leplatrem leplatrem merged commit 495c074 into mozilla-services:master Oct 22, 2015
@leplatrem leplatrem modified the milestone: 2.9 Oct 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants