sqlgrid icon indicating copy to clipboard operation
sqlgrid copied to clipboard

error: syntax error at or near "or" with CockroachDB

Open zllovesuki opened this issue 7 years ago • 13 comments

Hi, I was trying to use sqlgrid with CockroachDB (it speaks Postgres, but not fully implemented yet).

Though, when I tried to write a new file, it throws "error: syntax error at or near "or"" on this query:

CREATE 
OR REPLACE FUNCTION pg_temp.testfunc(OUT response "files_chunks", OUT sequelize_caught_exception text) RETURNS RECORD AS $ func_521f5503238b4f99a5847c9a66c8333c $ 
BEGIN
    INSERT INTO
        "files_chunks" ("id", "sha256", "data") 
    VALUES
        (
            DEFAULT, '<some very long string>'
        )
        RETURNING * INTO response;
EXCEPTION 
WHEN
    unique_violation 
THEN
    GET STACKED DIAGNOSTICS sequelize_caught_exception = PG_EXCEPTION_DETAIL;
END
$ func_521f5503238b4f99a5847c9a66c8333c $ LANGUAGE plpgsql;
SELECT
(testfunc.response).*,
    testfunc.sequelize_caught_exception 
FROM
    pg_temp.testfunc();
DROP FUNCTION IF EXISTS pg_temp.testfunc();

The dev team responded:

@zllovesuki: your client is trying to create a user-defined function (CREATE OR REPLACE FUNCTION). This feature is not yet implemented by CockroachDB.
@zllovesuki: we have a sequelize adapter which works around this problem by using the CockroachDb-specific UPSERT statement instead of this function to detect and handle conflicts

Perhaps you can modify the code and make it more "generic"? e.g. manually start a transaction?

fyi:

  1. https://www.cockroachlabs.com/docs/stable/sql-feature-support.html
  2. https://github.com/cockroachdb/sequelize-cockroachdb

zllovesuki avatar Apr 09 '18 17:04 zllovesuki

Thanks for the feedback.

You might be able to work around this by passing your own instance of sequelize to the sqlgrid constuctor.

If you try that, please let me know how that works

internalfx avatar Apr 09 '18 18:04 internalfx

I forked your module and I changed writeChunk into this:

let writeChunk = async function(file, num, data) {
  let hash = crypto.createHash('sha256').update(data).digest('hex')

  let chunk = {}

  let t = await sql.transaction()

  try {
    // https://github.com/sequelize/sequelize/issues/8628
    try {
      chunk = await chunks.create({
        sha256: hash,
        data: data
      }, {
        transaction: t
      })
    } catch (e) {
      chunk = await chunks.findOne({
        where: {
          sha256: hash
        }
      }, {
        transaction: t
      })
    }
    await t.commit()
  } catch (e) {
    await t.rollback()
  }

  await pointers.create({
    num: num,
    file_id: file.id,
    chunk_id: chunk.id
  })
}

It's not perfect (such as error handling), but at least it doesn't use FUNCTION

zllovesuki avatar Apr 10 '18 01:04 zllovesuki

Your SQLGrid + CockroachDB actually makes an interesting project (object storage). CockroachDB is a SQL NoSQL database (Postgres dialect + transaction support but the underlying storage is key-value RocksDB, plus easy replication and scaling, like RethinkDB).

zllovesuki avatar Apr 10 '18 01:04 zllovesuki

Your SQLGrid + CockroachDB actually makes an interesting project.

Thank you! I am familiar with what CRDB is doing, it looks neat.

Did you try my earlier suggestion by chance?

internalfx avatar Apr 10 '18 03:04 internalfx

Also about your specific suggestion....

I assume you are saying I have to avoid any sequelize.findOrCreate() calls?

Do we know that there is no penalty for postgres users by going this route?

internalfx avatar Apr 10 '18 03:04 internalfx

It looks like Sequelize would default to using FUNCTION if you call findOrCreate, which is not yet supported by CockroachDB. However, to minimize the changes to the code, a transaction to do find if conflict on create is acceptable IMO.

~~CockroachDB does have UPSERT, however it does not support RETURNING.~~ UPDATE: see below

There's a penalty with my approach, that is, you have to make two calls (well four actually because we have transaction) to the database instead of just one with the function.

However you are free to ignore this issue, I'm sure that my use case is a very edge case...

zllovesuki avatar Apr 10 '18 04:04 zllovesuki

I really think you can sidestep the issue entirely if you instantiate SQLGrid with a sequelize-cockroach instance.....

var Sequelize = require('sequelize-cockroachdb')

var sequelize = new Sequelize('bank', 'maxroach', '', {
  dialect: 'postgres',
  port: 26257,
  logging: false
});

var SQLGrid = require('sqlgrid')

var bucket = SQLGrid(sequelize)

// initBucket creates tables and indexes if they don't exist, returns a promise.
bucket.initBucket().then(function () {
  // We are now ready to read and write files
})

I am interested in supporting cockroachDB, but I'd like to know if the above works first for your use case.

internalfx avatar Apr 10 '18 04:04 internalfx

Not really, it didn't work. When I clone your project, I first try replacing sequelize with sequelize-cockroachdb, and findOrCreate stills create FUNCTION for some reasons.

zllovesuki avatar Apr 10 '18 05:04 zllovesuki

That's unfortunate...

I also found this... https://github.com/cockroachdb/sequelize-cockroachdb/issues/15

I assume it's the same error you got?

internalfx avatar Apr 10 '18 05:04 internalfx

oh yes that's the exact same error.

zllovesuki avatar Apr 10 '18 06:04 zllovesuki

Interestingly,

@zllovesuki: regarding your question about the sequelize adapter. UPSERT does support RETURNING in CockroachDB 2.0 but the adapter was not updated since v1.x. We still need to do that. If you need it, feel free to ping @nvanbenschoten.

^ from gitter

zllovesuki avatar Apr 10 '18 18:04 zllovesuki

Great!

I would certainly prefer it be fixed upstream since that is better overall.

internalfx avatar Apr 10 '18 18:04 internalfx

@zllovesuki Any idea if this has been fixed upstream yet?

internalfx avatar Dec 15 '18 15:12 internalfx