Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

I looked at this patchset and it seemed natural to apply 0008 next
(adding work_mem to subscriptions).  Attached is Dilip's latest version,
plus my review changes.  This will break the patch tester's logic; sorry
about that.

What part of this change is what sets the process's
logical_decoding_work_mem to the given value?  I was unable to figure
that out.  Is it missing or am I just stupid?

Changes:
* the patch adds logical_decoding_work_mem SGML, but that has already
  been applied (cec2edfa7859); remove dupe.

* parse_subscription_options() comment says that it will raise an error if a
  caller does not pass the pointer for an option but option list
  specifies that option.  It does not really implement that behavior (an
  existing problem): instead, if the pointer is not passed, the option
  is ignored.  Moreover, this new patch continued to fail to handle
  things as the comment says.  I decided to implement the documented
  behavior instead; it's now inconsistent with how the other options are
  implemented.  I think we should fix the other options to behave as the
  comment says, because it's a more convenient API; if we instead opted
  to update the code comment to match the code, each caller would have
  to be checked to verify that the correct options are passed, which is
  pointless and error prone.

* the parse_subscription_options API is a mess.  I reordered the
  arguments a little bit; also change the argument layout in callers so
  that each caller is grouped more sensibly.  Also added comments to
  simplify reading the argument lists.  I think this could be fixed by
  using an ad-hoc struct to pass in and out.  Didn't get around to doing
  that, seems an unrelated potential improvement.

* trying to do own range checking in pgoutput and subscriptioncmds.c
  seems pointless and likely to get out of sync with guc.c.  Simpler is
  to call set_config_option() to verify that the argument is in range.
  (Note a further problem in the patch series: the range check in
  subscriptioncmds.c is only added in patch 0009).

* parsing integers using scanint8() seemed weird (error messages there
  do not correspond to what we want).  After a couple of false starts, I
  decided to rely on guc.c's set_config_option() followed by parse_int().
  That also has the benefit that you can give it units.

* psql \dRs+ should display the work_mem; patch failed to do that.
  Added.  Unit display is done by pg_size_pretty(), which might be
  different from what guc.c does, but I think it works OK.
  It's the first place where we use pg_size_pretty to show a memory
  limit, however.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services