Jobs are not processed in multi schema setup. Apartment + GoodJob ( post 1.1.2 ) about good_job HOT 13 CLOSED

@gadimbaylisahil Yikes! 😰 Thanks for flagging this! I've been curious about multi-database setups. And thank you for all your contributions too 🙏

To point you in the directions I'm thinking:

• The underlying SQL query that does the advisory locking has a problem. I added the guard clause you saw because I was seeing occasionally, when under heavy load, jobs would be fetched by the query that were not locked. It was fairly rare which is why I felt ok adding a guard clause and flagging it for later:

  good_job/lib/good_job/job.rb

  Lines 59 to 62 in fe2d5cb

  	unfinished.priority_ordered.only_scheduled.limit(1).with_advisory_lock do |good_jobs|
  	good_job = good_jobs.first
  	# TODO: Determine why some records are fetched without an advisory lock at all
  	break unless good_job&.owns_advisory_lock?

• Database connections are being lost/swapped/leaked. I spent a lot of time with that in #99
• GoodJob doesn't currently work with the Apartment Gem and the guard clause has made that obvious.

I'll dig into this too. Thank you for the example application.

from good_job.

Just to keep you in the loop with my debugging. I'm seeing a SQL error when running this:

irb(main):008:0> GoodJob::Job.all.advisory_locked
    GoodJob::Job Load (2.1ms) SELECT "public"."good_jobs".* FROM "public"."good_jobs" LEFT JOIN pg_locks ON pg_locks.locktype = 'advisory'
  AND pg_locks.objsubid = 1
  AND pg_locks.classid = ('x'||substr(md5('public.good_jobs' || "public.good_jobs"."id"::text), 1, 16))::bit(32)::int
  AND pg_locks.objid = (('x'||substr(md5('public.good_jobs' || "public.good_jobs"."id"::text), 1, 16))::bit(64) << 32)::bit(32)::int WHERE "pg_locks"."locktype" IS NOT NULL LIMIT $1
Traceback (most recent call last):
ActiveRecord::StatementInvalid (PG::UndefinedTable: ERROR:  missing FROM-clause entry for table "public.good_jobs")
LINE 3: ....classid = ('x'||substr(md5('public.good_jobs' || "public.go...

from good_job.

I think the issue so far is that this line is too naive when constructing valid sql from the table name and fails when the table name contains a schema:

e.g. this line "#{table_name}"."#{primary_key}"::text should process the table_name more.

from good_job.

I think the issue so far is that this line is too naive when constructing valid sql from the table name and fails when the table name contains a schema:

good_job/lib/good_job/lockable.rb

Lines 24 to 33 in fe2d5cb

	scope :joins_advisory_locks, (lambda do
	join_sql = <<~SQL
	LEFT JOIN pg_locks ON pg_locks.locktype = 'advisory'
	AND pg_locks.objsubid = 1
	AND pg_locks.classid = ('x'||substr(md5(:table_name || "#{table_name}"."#{primary_key}"::text), 1, 16))::bit(32)::int
	AND pg_locks.objid = (('x'||substr(md5(:table_name || "#{table_name}"."#{primary_key}"::text), 1, 16))::bit(64) << 32)::bit(32)::int
	SQL
	joins(sanitize_sql_for_conditions([join_sql, { table_name: table_name }]))
	end)

e.g. this line "#{table_name}"."#{primary_key}"::text should process the table_name more.

Nice find! Definitely one step closer.

By saying should process more what do you exactly mean? You mean that it should process it better?

from good_job.

By saying should process more what do you exactly mean? You mean that it should process it better?

Yes 😄

Currently it assumes that table_name is a simple string (e.g. good_jobs) whereas it looks like under Apartment it's public.good_jobs. It could be split on the period and then wrapped with quotes and reconstructed. But I also wonder if there is a more ActiveRecord/Arel way to do this.

from good_job.

Awesome, I will try to work on it today/tomorrow

On a side note:

In multi tenancy setup with Apartment it's advised to keep the jobs table only queried from the main 'public' schema. So regardless of to which schema we are checked it, it's always queried from and saved to public.

Config option in apartment.rb

  # Add any models that you do not want to be multi-tenanted, but remain in the global (public) namespace.
  # A typical example would be a Customer or Tenant model that stores each Tenant's information.
  #
  config.excluded_models = %w{ GoodJob::Job }

This eases processing of jobs as alternative is to checking out to a different schema and querying jobs. As jobs contain tenant name in their serialized parameters, during processing it's switched to that tenant by monkey patching ActiveJob.

class ActiveJob::Base
  class << self
    def execute(job_data)
      Apartment::Tenant.switch(job_data['tenant']) do
        super
      end
    end
  end

  def serialize
    super.merge('tenant' => Apartment::Tenant.current)
  end
end

This works fine with good_job ^.

I am wondering if I should apply the same to PgLocks table and keep excluded_models as well. However, it may not be related at all.

from good_job.

From staring at this more, it looks like the GoodJob::PgLock model is completely unused in the implementation. The only reference to pg_locks is directly in the sql of

...so the entire pg_locks.rb file could be deleted; I think I was only using it for debugging.

I want to get you unblocked as expediently as possible, so don't let this block: looking at the SQL, I think this also may be incompatible with Rails' built-in multidatabase support.

from good_job.

@bensheldon After preprocessing the table_name to get rid of .public my jobs are processing just fine now. I can open a PR and we can discuss there if we can go with an alternative way rather than split('.').last

I haven't used the multiple database feature of Rails, I am guessing issue will be related to having different connections? But it seems rather separate topic than the one in this ticket.

from good_job.

From staring at this more, it looks like the GoodJob::PgLock model is completely unused in the implementation. The only reference to pg_locks is directly in the sql of

good_job/lib/good_job/lockable.rb

Lines 24 to 33 in fe2d5cb

	scope :joins_advisory_locks, (lambda do
	join_sql = <<~SQL
	LEFT JOIN pg_locks ON pg_locks.locktype = 'advisory'
	AND pg_locks.objsubid = 1
	AND pg_locks.classid = ('x'||substr(md5(:table_name || "#{table_name}"."#{primary_key}"::text), 1, 16))::bit(32)::int
	AND pg_locks.objid = (('x'||substr(md5(:table_name || "#{table_name}"."#{primary_key}"::text), 1, 16))::bit(64) << 32)::bit(32)::int
	SQL
	joins(sanitize_sql_for_conditions([join_sql, { table_name: table_name }]))
	end)

...so the entire pg_locks.rb file could be deleted; I think I was only using it for debugging.

I want to get you unblocked as expediently as possible, so don't let this block: looking at the SQL, I think this also may be incompatible with Rails' built-in multidatabase support.

Here is the quick lockable.rb that made it work for me

https://gist.github.com/gadimbaylisahil/f94f6e8a4d363b00c8266c93fe7a3634

Would I face any side-effects with this you reckon?

from good_job.

I went searching through ActiveRecord and I think it would be better to use the adapter quoting methods: https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/Quoting.html

from good_job.

Oops, sorry I missed your earlier comment. Yes. Please open a PR 🎉

from good_job.

@gadimbaylisahil please let me know if the v1.2.4 fixes your problem. Thank you for the PR! 🎉

from good_job.

@gadimbaylisahil please let me know if the v1.2.4 fixes your problem. Thank you for the PR! tada

Amazing! All works now! Thanks for directions.

from good_job.