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

Too many DB requests to add inventory items to shipment #6842

Closed
edipox opened this Issue Oct 21, 2015 · 13 comments

Comments

Projects
None yet
6 participants
@edipox

edipox commented Oct 21, 2015

Hi,

When someone creates an order spree strikes the database a lot of times:

 SQL (1.8ms)  UPDATE "spree_shipments" SET "updated_at" = '2015-10-21 12:17:44.455894' WHERE "spree_shipments"."id" = $1  [["id", 2]]
 SQL (1.0ms)  UPDATE "spree_orders" SET "updated_at" = '2015-10-21 12:17:44.461069' WHERE "spree_orders"."id" = $1  [["id", 1]]
 SQL (0.4ms)  INSERT INTO "spree_inventory_units" ("order_id", "variant_id", "line_item_id", "state", "shipment_id", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id"  [["order_id", 1], ["variant_id", 1], ["line_item_id", 4], ["state", "on_hand"], ["shipment_id", 2], ["created_at", "2015-10-21 12:17:44.468094"], ["updated_at", "2015-10-21 12:17:44.468094"]]

Basically those requests for each "inventory item" in the order.

I have the following config to stop tracking inventory levels:

Spree.config do |config|
  # Uncomment to stop tracking inventory levels in the application
  config.track_inventory_levels = false
end

Is there any way to avoid all those DB requests but still save shipment information (most importantly the address) ?

@edipox

This comment has been minimized.

Show comment
Hide comment
@edipox

edipox Oct 21, 2015

This request is done thousands of times when an order is completed:

  SQL (0.5ms)  UPDATE "spree_inventory_units" SET "pending" = 'f', "updated_at" = '2015-10-21 13:04:37.224306' WHERE "spree_inventory_units"."id" = $1

Given that I won't manage inventory and stock. Is there any way to disable those requests and everything related to inventory and stock or at least to minimize it?

edipox commented Oct 21, 2015

This request is done thousands of times when an order is completed:

  SQL (0.5ms)  UPDATE "spree_inventory_units" SET "pending" = 'f', "updated_at" = '2015-10-21 13:04:37.224306' WHERE "spree_inventory_units"."id" = $1

Given that I won't manage inventory and stock. Is there any way to disable those requests and everything related to inventory and stock or at least to minimize it?

@jasonfb

This comment has been minimized.

Show comment
Hide comment
@jasonfb

jasonfb Oct 23, 2015

Contributor

I'm not sure what the consequences of that would be. The inventory unit system (not to be confused with stock items, which keeps track of how much stock you have) is integral to shipments being created correctly, and in order to move through all the correct states you would need to get your shipment & order to play along nicely without the correct inventory units in them.

Also, pending on that table is a TINYINT, so how is it that your code is inserting the single character f into it? That seems odd to me. Does that turn into false magically in some SQL way that I'm unaware of? I suppose it must if it works for you that way.

Contributor

jasonfb commented Oct 23, 2015

I'm not sure what the consequences of that would be. The inventory unit system (not to be confused with stock items, which keeps track of how much stock you have) is integral to shipments being created correctly, and in order to move through all the correct states you would need to get your shipment & order to play along nicely without the correct inventory units in them.

Also, pending on that table is a TINYINT, so how is it that your code is inserting the single character f into it? That seems odd to me. Does that turn into false magically in some SQL way that I'm unaware of? I suppose it must if it works for you that way.

@edipox

This comment has been minimized.

Show comment
Hide comment
@edipox

edipox Oct 23, 2015

@jasonfb Thank you for your reply. I will keep that in mind.

But I need to improve this anyway. Currently creating an order with 4000 (quantity) of products (inventory units) the checkout gets really slow due to those thousands of DB queries.

And yeah, that thing of using 'f' seems to be some PostgreSQL magic. I didn't change anything related to that. And even though it is veeery slow with 'huge' quantities, it still works.

edipox commented Oct 23, 2015

@jasonfb Thank you for your reply. I will keep that in mind.

But I need to improve this anyway. Currently creating an order with 4000 (quantity) of products (inventory units) the checkout gets really slow due to those thousands of DB queries.

And yeah, that thing of using 'f' seems to be some PostgreSQL magic. I didn't change anything related to that. And even though it is veeery slow with 'huge' quantities, it still works.

@jasonfb

This comment has been minimized.

Show comment
Hide comment
@jasonfb

jasonfb Oct 23, 2015

Contributor

Ah yes. (I use mysql, but once upon a time I was on a Postgres project so I am not entirely unfamiliar)

4000 item carts definitely need special scale attention, and yes I think killing inventory tracking will be a big part of it.

IN our story, customers were at one point unable to checkout with more than 12 line items in their cart because of some bloat we wrote in our promotion system (We fixed than and now the limit is upwards of 40-50)

Contributor

jasonfb commented Oct 23, 2015

Ah yes. (I use mysql, but once upon a time I was on a Postgres project so I am not entirely unfamiliar)

4000 item carts definitely need special scale attention, and yes I think killing inventory tracking will be a big part of it.

IN our story, customers were at one point unable to checkout with more than 12 line items in their cart because of some bloat we wrote in our promotion system (We fixed than and now the limit is upwards of 40-50)

@edipox

This comment has been minimized.

Show comment
Hide comment
@edipox

edipox Oct 26, 2015

Here is the super ugly temporary hack to at least avoid those thousands of DB queries:

Rails.application.config.to_prepare do

  Spree::Stock::Coordinator.class_eval do

    def initialize(order, inventory_units = nil)
      @order = order
      @inventory_units = inventory_units || Spree::Stock::InventoryUnitBuilder.new(order).one_unit_per_line_item
    end

  end

  Spree::Stock::InventoryUnitBuilder.class_eval do
    def one_unit_per_line_item
      @order.line_items.flat_map do |line_item|
        1.times.map do |i|
          @order.inventory_units.includes( variant: { product: {
                shipping_category: { shipping_methods: [:calculator, { zones: :zone_members }] }
          } } ).build( pending: true, variant: line_item.variant, line_item: line_item, order: @order )
        end
      end
    end
  end

end

So I will close this issue by now

edipox commented Oct 26, 2015

Here is the super ugly temporary hack to at least avoid those thousands of DB queries:

Rails.application.config.to_prepare do

  Spree::Stock::Coordinator.class_eval do

    def initialize(order, inventory_units = nil)
      @order = order
      @inventory_units = inventory_units || Spree::Stock::InventoryUnitBuilder.new(order).one_unit_per_line_item
    end

  end

  Spree::Stock::InventoryUnitBuilder.class_eval do
    def one_unit_per_line_item
      @order.line_items.flat_map do |line_item|
        1.times.map do |i|
          @order.inventory_units.includes( variant: { product: {
                shipping_category: { shipping_methods: [:calculator, { zones: :zone_members }] }
          } } ).build( pending: true, variant: line_item.variant, line_item: line_item, order: @order )
        end
      end
    end
  end

end

So I will close this issue by now

@edipox edipox closed this Oct 26, 2015

@ykka

This comment has been minimized.

Show comment
Hide comment
@ykka

ykka Jan 20, 2016

Thanks @edipox , have you experienced any issues with the code you pasted or has it worked ok for you?

Thanks!

ykka commented Jan 20, 2016

Thanks @edipox , have you experienced any issues with the code you pasted or has it worked ok for you?

Thanks!

@edipox

This comment has been minimized.

Show comment
Hide comment
@edipox

edipox Jan 20, 2016

@ykka Nope. It does exactly what the code shows: creates a single inventory unity per order's line item.

For example:
Given a variant ring. if you have a line item in your order with 400 rings, this will create a single inventory unit.

This affects shipment because you will have a single item to be shipped, but in my case it didn't matter.

edipox commented Jan 20, 2016

@ykka Nope. It does exactly what the code shows: creates a single inventory unity per order's line item.

For example:
Given a variant ring. if you have a line item in your order with 400 rings, this will create a single inventory unit.

This affects shipment because you will have a single item to be shipped, but in my case it didn't matter.

@cbilgili

This comment has been minimized.

Show comment
Hide comment
@cbilgili

cbilgili Aug 17, 2016

Contributor

Thank you @edipox, it saved the day. I just needed to add

Spree::Stock::Differentiator.class_eval do
def missing
[]
end
def missing?
false
end
end

to work.

Contributor

cbilgili commented Aug 17, 2016

Thank you @edipox, it saved the day. I just needed to add

Spree::Stock::Differentiator.class_eval do
def missing
[]
end
def missing?
false
end
end

to work.

@nrowegt

This comment has been minimized.

Show comment
Hide comment
@nrowegt

nrowegt Jan 3, 2017

@edipox That just made a huge speedup to an app of mine, thanks much!

nrowegt commented Jan 3, 2017

@edipox That just made a huge speedup to an app of mine, thanks much!

@edipox

This comment has been minimized.

Show comment
Hide comment
@edipox

edipox Jan 3, 2017

@nrowegt @cbilgili No problem guys! I'm happy to know that this was useful for someone else. BTW I think that what @cbilgili suggests would be very useful as well.

edipox commented Jan 3, 2017

@nrowegt @cbilgili No problem guys! I'm happy to know that this was useful for someone else. BTW I think that what @cbilgili suggests would be very useful as well.

@danielricecodes

This comment has been minimized.

Show comment
Hide comment
@danielricecodes

danielricecodes Jan 13, 2017

@edipox - what version of Spree was your solution meant for? I just tried this with Spree 3.1.1 and now when I try to checkout, the Checkout controller won't let me progress to the Payment step. Its kicking me back out to the Address step and the Order line item quantities have been reset to 1. This doesn't appear to work with the latest version of Spree.

Everything looked great when I was on the Delivery step, but when I pushed "Save and Continue" on the Delivery step, there's code in Spree's Checkout model/controller that did not like what the code above did.

danielricecodes commented Jan 13, 2017

@edipox - what version of Spree was your solution meant for? I just tried this with Spree 3.1.1 and now when I try to checkout, the Checkout controller won't let me progress to the Payment step. Its kicking me back out to the Address step and the Order line item quantities have been reset to 1. This doesn't appear to work with the latest version of Spree.

Everything looked great when I was on the Delivery step, but when I pushed "Save and Continue" on the Delivery step, there's code in Spree's Checkout model/controller that did not like what the code above did.

@edipox

This comment has been minimized.

Show comment
Hide comment
@edipox

edipox Jan 13, 2017

@danielricecodes I'm not sure, but as I can see in some old code I've found it was Spree 3.0.4. Notice that In that project I wasn't concerned about inventory management, and the only thing about shipments that I wanted to persist was the address, so for that specific situation that ugly temporary hack was ok

edipox commented Jan 13, 2017

@danielricecodes I'm not sure, but as I can see in some old code I've found it was Spree 3.0.4. Notice that In that project I wasn't concerned about inventory management, and the only thing about shipments that I wanted to persist was the address, so for that specific situation that ugly temporary hack was ok

@danielricecodes

This comment has been minimized.

Show comment
Hide comment
@danielricecodes

danielricecodes Jan 18, 2017

@edipox - I figured it out thanks with help from the folks in Spree's Slack Channel.

It took the code from both you and @cbilgili to make this work. I'm posting the combined solution to help anyone else who arrives at this issue.

Spree::Stock::Coordinator.class_eval do
  def initialize(order, inventory_units = nil)
    @order = order
    @inventory_units = inventory_units || Spree::Stock::InventoryUnitBuilder.new(order).one_unit_per_line_item
  end
end

Spree::Stock::InventoryUnitBuilder.class_eval do
  def one_unit_per_line_item
    @order.line_items.flat_map do |line_item|    
      @order.inventory_units.includes( variant: { product: {
        shipping_category: { shipping_methods: [:calculator, { zones: :zone_members }] }
        } } ).build( pending: true, variant: line_item.variant, line_item: line_item, order: @order )
    end
  end
end

Spree::Stock::Differentiator.class_eval do
  def missing; []; end;
  def missing?; false; end;
end

PS: I do plan on opening an issue with Spree Core because this is an obvious O(n) performance issue/bug (meaning, order processing time is directly proportional to the quantity of the order) that should be addressed. With this issue present in Spree's Core, it means no one can handle Bulk Orders with Spree. Alternatively, if the app I'm working on required handling returns or more complex shipping workflows, the fix above would not be viable.

danielricecodes commented Jan 18, 2017

@edipox - I figured it out thanks with help from the folks in Spree's Slack Channel.

It took the code from both you and @cbilgili to make this work. I'm posting the combined solution to help anyone else who arrives at this issue.

Spree::Stock::Coordinator.class_eval do
  def initialize(order, inventory_units = nil)
    @order = order
    @inventory_units = inventory_units || Spree::Stock::InventoryUnitBuilder.new(order).one_unit_per_line_item
  end
end

Spree::Stock::InventoryUnitBuilder.class_eval do
  def one_unit_per_line_item
    @order.line_items.flat_map do |line_item|    
      @order.inventory_units.includes( variant: { product: {
        shipping_category: { shipping_methods: [:calculator, { zones: :zone_members }] }
        } } ).build( pending: true, variant: line_item.variant, line_item: line_item, order: @order )
    end
  end
end

Spree::Stock::Differentiator.class_eval do
  def missing; []; end;
  def missing?; false; end;
end

PS: I do plan on opening an issue with Spree Core because this is an obvious O(n) performance issue/bug (meaning, order processing time is directly proportional to the quantity of the order) that should be addressed. With this issue present in Spree's Core, it means no one can handle Bulk Orders with Spree. Alternatively, if the app I'm working on required handling returns or more complex shipping workflows, the fix above would not be viable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment