Skip to content

Conversation

@pramsey
Copy link
Contributor

@pramsey pramsey commented Jan 18, 2018

This doesn't really fix anything on our side, since the existing code already avoids sequence name collisions, but it addresses the issue in #318. This would just allow older versions of ogr2ogr to avoid the bug fixed in 7203 where ogr2ogr fixates on having a serial sequence named <relname>_cartodb_id_seq. What exposed the error was

  • running an ogr2ogr data upload (works)
  • running it again (fails with errors about missing sequence)

It happens because after the first ogr2ogr upload, the table is cartodb'fyed, and the sequence name changes when cartodbfy does the copy step, so it ends up with a unique sequence name that is not the <relname>_cartodb_id_seq form ogr2ogr expects when running in -append mode, which seems to be the default for a not-first upload.

INTO relseq;
-- If it's the name we want, then rename it
IF relseq IS NOT NULL AND relseq = Format('%I.%I', destschema, destseq) THEN
PERFORM _CDB_SQL(Format('ALTER SEQUENCE %s RENAME TO %s_tmp', relseq, destseq), '_CDB_Rewrite_Table');
Copy link
Contributor

Choose a reason for hiding this comment

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

I must be missing something: it seems to me it's renaming the existing seq relseq to the desired destseq (_cartodb_id_seq) when it already has that very same name (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's moving %s_cartodb_id_seq to _cartodb_id_seq_tmp so that in the following steps, when the new table is created, it will be possible for the serial in cartodb_id to use %s_cartodb_id_seq as it's sequence name. Probably it would be best to recognize this case (sequence already there, and usable, even if the table is going to be re-written) as another case, but that would require larger changes below, to address what is a fairly minor corner case right now (the ogr2ogr upload issue).

Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason I was missing the _tmp 😅 thanks for the clarification!

@oriolbx
Copy link

oriolbx commented Mar 8, 2018

A client is experiencing this same issue when using ogr2ogr to have a ETL workflow to append data to the same table when importing with ogr2ogr. Having a message error like

CARTO: RunSQL Response:{"error":["relation \"<relname>_cartodb_id_seq\" does not exist"]}

ERROR 1: Error returned by server : relation "<relname>_cartodb_id_seq" does not exist

@rafatower
Copy link
Contributor

Thank you guys for the fix and review.

Aside from the merge, there's also release and deploy for users to enjoy it. @oriolbx can you please reference the issue here and add as candidate to the RT?

@oriolbx
Copy link

oriolbx commented Mar 9, 2018

We gave the affected user an alternative to install and build the version where there is the fix. The fix went into gdal 2.2, so they can work off a stable branch https://trac.osgeo.org/gdal/changeset/41287
and to download it. They need to check out svn checkout https://svn.osgeo.org/gdal/branches/2.2/ gdal-2.2-svn and build the library

@pramsey pramsey merged commit 7559151 into master Mar 9, 2018
@rafatower rafatower deleted the seq-drop branch March 12, 2018 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants