Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support with_meta without transactions #136

Closed
palkan opened this issue Sep 3, 2019 · 5 comments
Closed

Support with_meta without transactions #136

palkan opened this issue Sep 3, 2019 · 5 comments

Comments

@palkan
Copy link
Owner

palkan commented Sep 3, 2019

Problem

Currently, wrapping execution with Logidze.with_meta (or Logidze.with_responsible) call also wraps everything into a DB transaction, i.e.:

Logidze.with_meta(data) do
  do_something
end

# structurally equal to
ActiveRecord::Base.transaction do
  set_logidze_meta(data)
  do_something
  reset_logidze_meta
end

That could lead to an unexpected behavior, especially, when using .with_responsible within an around_action hook: all the DB operations happened within the action could be rollbacked in case of the exception. Thus, wrapping everything into .with_responsible significantly changes the way the application works.

Potential Solution

We need to investigate the possibility of removing the transaction from .with_meta. The reason why it was used is to guarantee the state at the end of the block at the DB side (that's the way SET LOCAL works in transactions).

If it's possible then we need to upgrade the current behavior by adding a transactional: true|false option for .with_meta/.with_responsible methods with true by default.

Also, we might consider adding a global switch (Logidze.transactional_meta = true | false).

@oleg-kiviljov
Copy link
Contributor

Hi @palkan, are there any ideas how this could be implemented?

@palkan
Copy link
Owner Author

palkan commented Nov 19, 2019

I have one idea which could described as follows:

# add an option to specify whether we want a transaction or not 
# (this is mostly for backward compatibility)
def with_meta(data, transactional: true, &block)
  #  transactional_with_meta is the current implementation
  return MetaTransaction.wrap(data, &block) if transactional

  MetaNoTransaction.wrap(data, &block)
end

# the same as current MetaTransaction with the only one change
class Meta
  def perform
    raise ArgumentError, "Block must be given" unless block

    call_block_in_meta_context
  end

  def call_block_in_meta_context
    prev_meta = current_meta

    meta_stack.push(meta)

    pg_set_meta_param(current_meta)
    result = block.call

    result
  ensure
    # That's the only difference: we move reset to ensure to call it in case of exceptions
    pg_reset_meta_param(prev_meta)
    meta_stack.pop
  end
end

class MetaTransaction < BaseMeta
  def  call_block_in_meta_context
    ActiveRecord::Base.transaction { super }
  end
end

I hope, that should be enough)

@oleg-kiviljov
Copy link
Contributor

Hi @palkan, I don't really get how the SET LOCAL is done without transaction based on your example.

@palkan
Copy link
Owner Author

palkan commented Nov 25, 2019

Yeah, there is the point missing; when we're not within a transaction, we can only use SET (w/o LOCAL); and that's why we have to cleanup manually.

@palkan
Copy link
Owner Author

palkan commented Nov 29, 2019

Closed by #143

@palkan palkan closed this as completed Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants