Truncate large names, with an opaleye tag to avoid collisions

[cristovaogferreira-artificial]

See issue #366.

This PR adds a function selectTruncated that limits the alias names to a specific size (> 29 characters), using SHA1 to generate a unique (modulo SHA1 collisions and a column with a deliberately colliding name) suffix.

This allows us to make Postgres happy about column name size while at the same time keeping some debug information around.

[TeofilC]

Did you see this comment? #366 (comment) My impression is that this is already fixed, since we truncate and then add a unique tag

[cristovaogferreira-artificial]

Did you see this comment? #366 (comment) My impression is that this is already fixed, since we truncate and then add a unique tag

I did see the comment, it doesn't fix another issue which is select aliases still being generated with too long names (we still see a bunch of warnings in our service, that's why I have been working on this). See the tests for details.

(edit: I realized the tests don't show the behaviour, because they're using a 42 chars identifier, but it's there. I'll add another test.)

[TeofilC]

Would it be possible to take the same approach as before here too then, ie, use a Opaleye.fresh? Rather than using SHA1?

[cristovaogferreira-artificial]

Would it be possible to take the same approach as before here too then, ie, use a Opaleye.fresh? Rather than using SHA1?

Done

[TeofilC]

Could you use the same helper here:

rel8/src/Rel8/Statement.hs

Line 198 in 95ec308

suffix = Opaleye.tagWith tag (Opaleye.tagWith subtag "")

to avoid duplication.

Also is it necessary to make the size user configurable? I think using the same constant as before would be good, and then we wouldn't need to add extra stuff to the interface?

[cristovaogferreira-artificial]

Could you use the same helper here:

rel8/src/Rel8/Statement.hs

Line 198 in 95ec308

suffix = Opaleye.tagWith tag (Opaleye.tagWith subtag "")

to avoid duplication.

Just to be sure: You mean extract the helper from that function and using that, right? Something like

statementReturning :: Table Expr a 
  => State Opaleye.Tag Doc -> Statement (Query a)
statementReturning pp = Statement $ do
  (binding, query) <- lift $ do
    doc <- pp
    tag <- Opaleye.fresh
    let
      relation = Opaleye.tagWith tag "statement"
      names = namesFromLabelsWithA (symbol tag) `evalState` Opaleye.start
      columns = Just $ showNames names
      query =
        fromCols <$> each
          TableSchema
            { name = fromString relation
            , columns = names
            }
      returning = Returning (countRows query)
      binding = Binding {..}
    pure (binding, query)
  tell (Endo (binding :))
  pure $ Unmodified query

symbol :: Tag -> NonEmpty String -> State Opaleye.Tag String
symbol tag labels = do
  subtag <- Opaleye.fresh
  let
    suffix = Opaleye.tagWith tag (Opaleye.tagWith subtag "")
  pure $ take (63 - length suffix) label ++ suffix
  where
    label = fold (intersperse "/" labels)

and use symbol in the implementation of a namesFromLabelsTruncated?

Also is it necessary to make the size user configurable? I think using the same constant as before would be good, and then we wouldn't need to add extra stuff to the interface?

That's fine, 63 seems to be the number in most postgres implementations that people are worried about.

[TeofilC]

Yep that's the sort of thing. I'm not sure if these are exactly the same, but they seem quite similar. Whatever you think is best

[cristovaogferreira-artificial]

Done. I also renamed selectTruncated to selectShort because "truncate" is a very overloaded term in DB-land.

[TeofilC]

Sorry one last thing. Could you just make this the default (or maybe there's a reason not to?)? So don't change the interface of Rel8 at all on this PR.

[cristovaogferreira-artificial]

Sorry one last thing. Could you just make this the default (or maybe there's a reason not to?)? So don't change the interface of Rel8 at all on this PR.

Two concerns:

• Is it possible that someone actually changed their postgres installation to keep long names possible for debugging reasons? If so this change would break their usage.

  • My guess is "no-one is", because it requires a recompile with a config change, and even if it were "yes" anyone who's recompiling postgres probably doesn't shy away from patching and recompiling rel8, but you know the consumers of the library better than me.

• Do I change the top-level namesFromLabels function, then?

  • What is the expectation of someone calling namesFromLabels, is it that it does the same scheme that Rel8 uses internally?
  • Even if the expectation is just "generate some names that are good to use as postgres column names", it should change.

[TeofilC]

I agree that it's unlikely that someone will change the default length.

Yeah I think the nameFromLabels function should match internal usage.

[cristovaogferreira-artificial]

Done. Let me know if there's anything else!

[TeofilC]

Looks great thanks!

I'll try to cut a release soon btw