Re: PATCH: logical_work_mem and logical streaming of large in-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